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(traceparent): setting parent span from server #477

Merged
merged 13 commits into from
Nov 8, 2019

Conversation

obecny
Copy link
Member

@obecny obecny commented Nov 4, 2019

Which problem is this PR solving?

  1. Fixes Allow linking the server-side HTML render with client side HTML fetch spans #457

Short description of the changes

It adds functionality of reading the server span using the trace context. It will allow to share context between server and client.

Copy link
Member

@bg451 bg451 left a comment

Choose a reason for hiding this comment

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

Awesome, should add documentation somewhere that this is something you cna do, similar to the link census docs.

@codecov-io
Copy link

codecov-io commented Nov 4, 2019

Codecov Report

Merging #477 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #477      +/-   ##
==========================================
+ Coverage    92.5%   92.66%   +0.16%     
==========================================
  Files         136      137       +1     
  Lines        6511     6627     +116     
  Branches      593      587       -6     
==========================================
+ Hits         6023     6141     +118     
+ Misses        488      486       -2
Impacted Files Coverage Δ
...y-core/src/context/propagation/HttpTraceContext.ts 100% <100%> (ø) ⬆️
...telemetry-plugin-document-load/src/documentLoad.ts 98.09% <100%> (+0.05%) ⬆️
...es/opentelemetry-node/src/instrumentation/utils.ts 90.47% <0%> (-9.53%) ⬇️
...opentelemetry-base/test/resources/resource.test.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/src/config.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/src/utility.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/test/Span.test.ts 100% <0%> (ø) ⬆️
.../opentelemetry-plugin-dns/test/utils/assertSpan.ts 100% <0%> (ø) ⬆️
...ry-tracing/test/export/SimpleSpanProcessor.test.ts 100% <0%> (ø) ⬆️
...try-node/test/instrumentation/PluginLoader.test.ts 100% <0%> (ø) ⬆️
... and 10 more

@obecny
Copy link
Member Author

obecny commented Nov 4, 2019

Awesome, should add documentation somewhere that this is something you cna do, similar to the link census docs.

@bg451 updated readme

Copy link
Contributor

@draffensperger draffensperger left a comment

Choose a reason for hiding this comment

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

👍 Nice!!

@@ -76,12 +84,17 @@ export class DocumentLoad extends BasePlugin<unknown> {
* Collects information about performance and creates appropriate spans
*/
private _collectPerformance() {
const windowWithTrace: WindowWithTrace = (window as unknown) as WindowWithTrace;
const serverContext =
parseTraceParent(windowWithTrace.traceparent || '') || undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that parseTraceParent already will return null if it fails to parse, would this still work without the || undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

the undefined must be there as the SpanOptions.parent is optional and cannot be null so the typescript is complaining about it.

@dyladan
Copy link
Member

dyladan commented Nov 6, 2019

I'm sure it's much too late to bring something like this up but just for my own edification, why did you choose to embed the trace parent in a script tag with a variable as opposed to a meta tag like <meta name="traceparent" content="00-ab42124a3c573678d4d8b21ba52df3bf-d21f7bc17caa5aba-01"> like is common for things like csrf tokens? This would ensure it is available immediately for script execution regardless of script execution order, and would work in cases where things like CORS rules prevent certain JS scripts from running.

@obecny
Copy link
Member Author

obecny commented Nov 6, 2019

I'm sure it's much too late to bring something like this up but just for my own edification, why did you choose to embed the trace parent in a script tag with a variable as opposed to a meta tag like <meta name="traceparent" content="00-ab42124a3c573678d4d8b21ba52df3bf-d21f7bc17caa5aba-01"> like is common for things like csrf tokens? This would ensure it is available immediately for script execution regardless of script execution order, and would work in cases where things like CORS rules prevent certain JS scripts from running.

It is never too late to improve things :).
I think this is really a valid point. The implementation is similar as for opencensus. But I think it might make more sense to put it in meta as you suggested. @draffensperger what do you think about it ?

@dyladan
Copy link
Member

dyladan commented Nov 6, 2019

If it isn't too late to change, I prefer the meta option. In addition to being outside of script execution, it is also more reliable and simplifies server-side implementation as HTML is much easier to parse/modify without unexpected side effects than JS is.

@obecny
Copy link
Member Author

obecny commented Nov 6, 2019

@dyladan you might have a look again, I changed the traceparent to be read from meta tag

@draffensperger
Copy link
Contributor

Good idea about the meta tag! The reason it was in a <script> was just that we did it that way in OpenCensus and that was just because I didn't know about the value of meta tags :)

I think in the future it could potentially also allow it to be set via the Server Timing header, which is the only header that the JS client can read directly from the initial page load. That would allow something like a load balancer to inject it without changing the HTML. But that's a future improvement and should be optional!

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks for making the meta tag change.

@@ -25,7 +25,17 @@ const VALID_SPANID_REGEX = /^[0-9a-f]{16}$/i;
const INVALID_ID_REGEX = /^0+$/i;
const VERSION = '00';

function parse(traceParent: string): SpanContext | null {
/**
* Parses information from [traceParent] window property and converts it into {@link SpanContext}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Parses information from [traceParent] window property and converts it into {@link SpanContext}
* Parses information from the [traceparent] span tag and converts it into {@link SpanContext}

@dyladan
Copy link
Member

dyladan commented Nov 8, 2019

Good idea about the meta tag! The reason it was in a <script> was just that we did it that way in OpenCensus and that was just because I didn't know about the value of meta tags :)

I think in the future it could potentially also allow it to be set via the Server Timing header, which is the only header that the JS client can read directly from the initial page load. That would allow something like a load balancer to inject it without changing the HTML. But that's a future improvement and should be optional!

Additionally, the trace context working group is going to be working on a trace context response header for this use specifically

@mayurkale22 mayurkale22 merged commit 4f48357 into open-telemetry:master Nov 8, 2019
@obecny obecny deleted the document-load-traceparent branch July 8, 2020 12:13
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.

Allow linking the server-side HTML render with client side HTML fetch spans
7 participants