Skip to content

Add spec for Span Link and Span Continuation #600

Merged
gregkalapos merged 8 commits intoelastic:mainfrom
gregkalapos:SpanLinks
Apr 28, 2022
Merged

Add spec for Span Link and Span Continuation #600
gregkalapos merged 8 commits intoelastic:mainfrom
gregkalapos:SpanLinks

Conversation

@gregkalapos
Copy link
Copy Markdown
Contributor

@gregkalapos gregkalapos commented Feb 22, 2022

This PR specifies Span links and Span Continuation. The first is about introducing links between spans (and transactions), the second handles incoming HTTP requests with traceparent headers from outside Elastic APM.

Solves: #596

  • May the instrumentation collect sensitive information, such as secrets or PII (ex. in headers)?
    • No
      • There won't be any new information collected. The agent will introduce a new array which will contain transaction/span ids and trace ids. This only introduces links between things which the agent already collects.
  • Link to meta issue: https://github.com/elastic/observability-dev/issues/1947 related issue: Traceparent header from outside Elastic #286
  • Create PR as draft
  • Approval by at least one other agent
  • Mark as Ready for Review (automatically requests reviews from all agents and PM via CODEOWNERS)
    • Remove PM from reviewers if impact on product is negligible
    • Remove agents from reviewers if the change is not relevant for them
  • Approved by at least 2 agents + PM (if relevant)
  • Merge after 7 days passed without objections
  • Create implementation issues (ideally add a milestone)
  • Create a status table and add it to the meta issue

Specification for span links and trace continuation
@gregkalapos
Copy link
Copy Markdown
Contributor Author

Open questions:

  • Where do we put this?
    Currently it’s a new file, other throughs I had:

    • tracing-spans.md and tracing-transactions.md: Since this applies to both spans and transactions I think putting into any of those would be confusing and may suggest that this only applies to one of them.
    • tracing-distributed-tracing.md: especially with OTel, this isn’t necessarily distributed tracing.
      So my first idea is to add a new file .
  • At this point labels aren’t handled here. Should we add those as well or take care of those later? At this point it’s not yet done in APM Server.

@ghost
Copy link
Copy Markdown

ghost commented Feb 22, 2022

💚 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: 2022-04-26T17:14:07.923+0000

  • Duration: 3 min 18 sec

Copy link
Copy Markdown
Member

@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.

  • Where do we put this?
    +1 to put it in a separate file
  • At this point labels aren’t handled here. Should we add those as well or take care of those later? At this point it’s not yet done in APM Server.

It's fine to add support for span link labels later, I think.
I don't really know what they're used for tbh. Could you do some research to find out in which situations one might use them and how important these use cases are?

gregkalapos and others added 2 commits February 22, 2022 15:04
Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>
@felixbarny felixbarny linked an issue Feb 28, 2022 that may be closed by this pull request
@basepi
Copy link
Copy Markdown
Contributor

basepi commented Feb 28, 2022

Could span link attributes (as supported by otel) be useful for metadata around the link? Perhaps for differentiating links sourced from external traceparent headers vs those added manually or for batching?

@SergeyKleyman
Copy link
Copy Markdown
Contributor

I have minor suggestion regarding the naming: trace_continuation_strategy instead of span_continuation_strategy since it controls whether agent continues or breaks a trace. WDYT?

@felixbarny
Copy link
Copy Markdown
Member

This PR proposes trace_continuation_strategy. If I wrote span_continuation_strategy somewhere else, that's a typo. The spec is the single source of truth.

Copy link
Copy Markdown
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion to link between the docs, now that they are separated.

gregkalapos and others added 2 commits March 24, 2022 19:51
Co-authored-by: Trent Mick <trentm@gmail.com>
@gregkalapos gregkalapos self-assigned this Mar 24, 2022
@gregkalapos
Copy link
Copy Markdown
Contributor Author

Summary from a zoom discussion

  • Attributes (aka tags or labels): The OTel API offers a way to add attributes to each span link. Every span link has a list of key value pairs. So far we haven't seen any real world use case for using these, so we decided to not add support for it at this point. Without real use cases it's hard to answer questions like: "how should the UI look like for span attributes?" This can be revisited later and we can easily extend our spec to accommodate this. @basepi had a question above which could have been covered by supporting attributes, so the answer to that with this decision is that we won't cover it in the first iteration.
  • Project id described in this comment: Same decision - won't be in the first iteration, but we can add it later. Main reason here is that it'd increase complexity significantly (each agent should properly test it) with a very limited benefit for very specific use cases. Plus we can add it easily later.

@gregkalapos gregkalapos marked this pull request as ready for review April 7, 2022 15:04
@gregkalapos gregkalapos requested review from a team as code owners April 7, 2022 15:04
@gregkalapos gregkalapos requested review from a team as code owners April 7, 2022 15:04
@gregkalapos
Copy link
Copy Markdown
Contributor Author

We discussed this with @felixbarny - agreed to merge this now in order to unblock some other issues and not wait 7 days this time.

@gregkalapos gregkalapos merged commit ad59bb5 into elastic:main Apr 28, 2022
@gregkalapos gregkalapos deleted the SpanLinks branch April 28, 2022 15:47
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.

Create agent spec for span continuation strategy

8 participants