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

feat(user-interaction): adding traceparent context from meta tag #1850

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

xoscar
Copy link

@xoscar xoscar commented Dec 7, 2023

Which problem is this PR solving?

  • Each user interaction creates a new trace ID instead of using the one from the traceparent if exists

Short description of the changes

  • Adds a function to retrieve the context based on the traceparent header meta tag
  • Useful to generate a single trace for the entire user session

@xoscar xoscar requested a review from a team December 7, 2023 16:31
@xoscar xoscar changed the title feat: adding traceparent from meta tag feat: adding traceparent context from meta tag Dec 7, 2023
@dyladan dyladan changed the title feat: adding traceparent context from meta tag feat(user-interaction): adding traceparent context from meta tag Dec 7, 2023
Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.42%. Comparing base (dfb2dff) to head (2662689).
Report is 190 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1850      +/-   ##
==========================================
- Coverage   90.97%   90.42%   -0.56%     
==========================================
  Files         146      148       +2     
  Lines        7492     7333     -159     
  Branches     1502     1524      +22     
==========================================
- Hits         6816     6631     -185     
- Misses        676      702      +26     

see 62 files with indirect coverage changes

@JamieDanielson
Copy link
Member

Sorry for the delay on this @xoscar , we are catching up on PRs. I am wondering if the existing behavior is this way for a specific reason, and also curious about whether this change would benefit from being a configuration option. This also may change with the new instrumentation written in events, so it's not clear whether this is useful to implement now or consider something like this for the upcoming instrumentation change. Either way it would be good to get some input from some folks who work more on the web instrumentation. @martinkuba @MSNev @pkanal do you have any thoughts on this?

@xoscar
Copy link
Author

xoscar commented Jun 26, 2024

Hello @JamieDanielson sorry for the late response.

I'm currently working at Tracetest and we have provided a library that extends the current user interaction behavior using a single trace.
This is particularly useful when running synthetic tests from the browser side (playwright/cypress/etc) as a test can be encapsulated in a single trace that propagates the context from the browser side to the backend services.

Allowing users to create assertions against user-triggered events to backend spans.

I understand that the official implementation my diverge from what we currently have but we still think this has a valuable use case.

Thanks!

@JamieDanielson
Copy link
Member

I'm currently working at Tracetest and we have provided a library that extends the current user interaction behavior using a single trace.
This is particularly useful when running synthetic tests from the browser side (playwright/cypress/etc) as a test can be encapsulated in a single trace that propagates the context from the browser side to the backend services.

Oh, interesting use case, thank you for the details. I do wonder if it should be a configuration option, but don't want to ask you to do that work if this isn't something desired here. Pinging @martinkuba @MSNev @pkanal again for thoughts 🙏

@MSNev
Copy link
Contributor

MSNev commented Jul 24, 2024

@JamieDanielson
Copy link
Member

We discussed this a bit during the JS SIG meeting today. Generally there was hesitation in taking this on, because it seems the instrumentation is actually working as expected right now with the user interaction being the root span, and this change would be breaking. With the expected changes / new instrumentation based on events coming up, we're not sure about having two major changes here. Also Nev linked a spec issue above where we are exploring options to standardize on how this may be done more holistically, and that work/discussion is still underway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants