Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling huge tracing specs #453

Merged
merged 50 commits into from
Jul 19, 2021
Merged

Conversation

felixbarny
Copy link
Member

@felixbarny felixbarny commented Jun 21, 2021

Preview

Closes #449

POC implementation for span compression: https://github.com/felixbarny/apm-agent-java/tree/compressed-spans

@apmmachine
Copy link

apmmachine commented Jun 21, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-07-19T11:57:56.131+0000

  • Duration: 3 min 53 sec

  • Commit: 6821501

Trends 🧪

Image of Build Times

Copy link
Member

@AlexanderWert AlexanderWert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Spec so far!

General comment: How about keeping this Spec in one file to avoid getting lost in too many Spec files in general?

specs/agents/tracing-spans-dropped-stats.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-drop-fast.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-handling-huge-traces.md Outdated Show resolved Hide resolved
Copy link
Member Author

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about keeping this Spec in one file to avoid getting lost in too many Spec files in general?

There are really multiple specs for one theme in a single PR. I'd like to separate different specs in different files. I'd expect the main navigation not to be the GitHub file viewer but the README.md. If it makes things easier, we could even add a breadcrumb to the header of each spec.

specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-dropped-stats.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-dropped-stats.md Outdated Show resolved Hide resolved
Copy link
Contributor

@SergeyKleyman SergeyKleyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks very good for just a few minor comments.

specs/agents/tracing-spans-handling-huge-traces.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-limit.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-dropped-stats.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-dropped-stats.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
@SergeyKleyman SergeyKleyman requested review from a team as code owners July 15, 2021 09:00
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-dropped-stats.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-compress.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-drop-fast-exit.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-drop-fast-exit.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans-dropped-stats.md Outdated Show resolved Hide resolved
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Suggested an alternative wording, feel free to ignore

specs/agents/tracing-spans-drop-fast-exit.md Outdated Show resolved Hide resolved
@SergeyKleyman
Copy link
Contributor

SergeyKleyman commented Jul 19, 2021

  • ... and exact_match: The only usage we discussed is for single trace view to give user visual cues about multiple spans compressed into just one

@estolfo As we discussed it actually might be useful to index compression_strategy (a new property that replaced exact_match)

specs/agents/tracing-spans-destination.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans.md Outdated Show resolved Hide resolved
specs/agents/tracing-spans.md Outdated Show resolved Hide resolved
@AlexanderWert AlexanderWert enabled auto-merge (squash) July 19, 2021 11:58
@AlexanderWert AlexanderWert merged commit e528576 into elastic:master Jul 19, 2021
@apmmachine
Copy link

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-07-19T11:57:50.252+0000

  • Duration: 3 min 1 sec

  • Commit: 6821501

Trends 🧪

Image of Build Times

trentm added a commit to trentm/apm that referenced this pull request Oct 6, 2021
* First draft of handling huge tracing specs

* Apply suggestions from code review

Co-authored-by: Alexander Wert <[email protected]>

* Implement suggestions

* Update specs/agents/tracing-spans-compress.md

Co-authored-by: Alexander Wert <[email protected]>

* Pseudo code for how the strategies work in combination

* Add composite.exact_match flag

* Apply suggestions from code review

Co-authored-by: Colton Myers <[email protected]>

* Add breadcrumbs

* Add missing table of contents link to AWS tracing spec file

* Some clarifications for the destination APIs (elastic#452)

* Add limit to dropped_spans_stats

* Add implementation section to transaction_max_spans

* Move exit span definition from destination spec to span spec

* Add exit_span_min_duration spec

* Apply suggestions from code review

Co-authored-by: Sergey Kleyman <[email protected]>

* Fix links, add clarification to max duration

* Dropping fast spans requires stats

* Rework transaction_max_spans implementation logic

* Improve transaction_max_spans: no CAS

* Apply suggestions from code review

Co-authored-by: Sergey Kleyman <[email protected]>

* Update specs/agents/tracing-spans-compress.md

* Update specs/agents/tracing-spans-compress.md

* Update specs/agents/tracing-spans-handling-huge-traces.md

Co-authored-by: Trent Mick <[email protected]>

* Renamed same_kind_compression_max_duration config option

to span_compression_same_kind_max_duration

* Added span_compression_same_kind_max_duration config option

* Added span_compression_enabled config option

* Update specs/agents/tracing-spans-compress.md

Co-authored-by: eyalkoren <[email protected]>

* Changed end to sum.us in composite sub-object

* Replaced exact_match bool with compression_strategy enum

* Update specs/agents/tracing-spans-compress.md

Co-authored-by: eyalkoren <[email protected]>

* Added outcome requirement to eligible for compression

* Added outcome requirement to eligible for compression PART 2

Updated isCompressionEligible() pseudo-code

* Added links from tracing-spans.md to tracing-spans-compress.md

* Fixed missing isSameKind check in tryToCompressComposite()

* Update specs/agents/tracing-spans-drop-fast-exit.md

Co-authored-by: Alexander Wert <[email protected]>

* Update specs/agents/tracing-spans-compress.md

Co-authored-by: Alexander Wert <[email protected]>

* Update specs/agents/tracing-spans-drop-fast-exit.md

Co-authored-by: Alexander Wert <[email protected]>

* Update specs/agents/tracing-spans-compress.md

Co-authored-by: Alexander Wert <[email protected]>

* Update specs/agents/tracing-spans-compress.md

Co-authored-by: Alexander Wert <[email protected]>

* Update specs/agents/tracing-spans-compress.md

Co-authored-by: Alexander Wert <[email protected]>

* Removed "Exit span API" requirement from tracing-spans.md

* Update specs/agents/tracing-spans-drop-fast-exit.md

Co-authored-by: eyalkoren <[email protected]>

* reafctored file structure for handling huge traces

* Update specs/agents/tracing-spans-destination.md

* Update specs/agents/tracing-spans.md

* Update specs/agents/tracing-spans.md

Co-authored-by: Alexander Wert <[email protected]>
Co-authored-by: Colton Myers <[email protected]>
Co-authored-by: Trent Mick <[email protected]>
Co-authored-by: eyalkoren <[email protected]>
Co-authored-by: Sergey Kleyman <[email protected]>
Co-authored-by: Trent Mick <[email protected]>
Co-authored-by: Alexander Wert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[META 432] SPEC + Reference implementation for compressed spans algorithm