Skip to content
This repository has been archived by the owner on Jul 10, 2023. It is now read-only.

Add Http Trace Propagation and implement extract and inject methods in ThundraTracer #40

Merged
merged 5 commits into from Dec 13, 2018

Conversation

kobalski
Copy link
Contributor

No description provided.


_extractBaggageItems(carrier: any, spanContext: ThundraSpanContext) {
Object.keys(carrier).forEach((key) => {
const pattern = new RegExp(`^${TextMapPropagator.BAGGAGE_PREFIX}(.+)$`);
Copy link
Member

Choose a reason for hiding this comment

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

might be constant at the TextMapPropagator level so there won't be parse for the regexp itself every time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

if (!HttpIntegration.isValidUrl(host)) {
return request.apply(this, [options, callback]);
Copy link
Member

Choose a reason for hiding this comment

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

I think, in case of invalid url that we are not interested in, headers should not be passed in the options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, if the url is invalid we do not create span and inject any headers.

@coveralls
Copy link

coveralls commented Dec 13, 2018

Pull Request Test Coverage Report for Build 240

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 65 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.7%) to 65.277%

Files with Coverage Reduction New Missed Lines %
dist/plugins/Trace.js 8 88.72%
dist/opentracing/Span.js 12 75.86%
dist/opentracing/Tracer.js 13 76.92%
dist/plugins/integrations/HttpIntegration.js 32 11.11%
Totals Coverage Status
Change from base Build 234: -0.7%
Covered Lines: 1400
Relevant Lines: 1975

💛 - Coveralls

className: ClassNames.LAMBDA,
});

this.rootSpan.spanContext.parentId = propagatedSpanContext.spanId;
Copy link
Member

Choose a reason for hiding this comment

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

Can't this be done by passing span context to _startSpan here? (Passing traceId, transactionId, parentId and baggageItems at once in the span context itself) So we don't need to modify span context after created it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, added parentContext option to _startSpan method.

@serkan-ozal serkan-ozal merged commit 290900d into master Dec 13, 2018
@kobalski kobalski deleted the feature/trace-propagation branch December 13, 2018 14:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants