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: introduces ability to suppress tracing via context #1344

Merged
merged 19 commits into from
Aug 25, 2020

Conversation

michaelgoin
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Introduced helpers for getting/setting the new 'suppressInstrumentation' value.
  • Updated Tracer to automatically stop span generation, via cached no-op span, when suppressInstrumentation true.
  • Updated span processors to set context to suppress instrumentation prior to invoking exporters.

I did not tackle updating usages of the header the currently exist. Seemed like those might be better left separate to do in small/safe steps once the mechanism is in place.

There didn't seem to be any obvious/great place for a real-world integration test of this. As such, I introduced some tests that best try to simulate the problem being solved via test helper classes. Its a bit funky but I think captures the problem in an automated fashion. Open to feedback if there are known better approaches.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 23, 2020

CLA Check
The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jul 23, 2020

Codecov Report

Merging #1344 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1344      +/-   ##
==========================================
+ Coverage   93.78%   93.80%   +0.02%     
==========================================
  Files         149      149              
  Lines        4524     4539      +15     
  Branches      939      940       +1     
==========================================
+ Hits         4243     4258      +15     
  Misses        281      281              
Impacted Files Coverage Δ
packages/opentelemetry-core/src/context/context.ts 95.83% <100.00%> (+1.71%) ⬆️
packages/opentelemetry-tracing/src/Tracer.ts 100.00% <100.00%> (ø)
...telemetry-tracing/src/export/BatchSpanProcessor.ts 92.45% <100.00%> (+0.29%) ⬆️
...lemetry-tracing/src/export/InMemorySpanExporter.ts 100.00% <100.00%> (ø)
...elemetry-tracing/src/export/SimpleSpanProcessor.ts 85.00% <100.00%> (+2.64%) ⬆️

@michaelgoin michaelgoin force-pushed the suppress-instrumentation branch from 526a08e to 98acb74 Compare July 23, 2020 19:48
packages/opentelemetry-core/src/context/context.ts Outdated Show resolved Hide resolved
packages/opentelemetry-core/src/context/context.ts Outdated Show resolved Hide resolved
packages/opentelemetry-tracing/src/Tracer.ts Outdated Show resolved Hide resolved
import { ContextManager, Context } from '@opentelemetry/context-base';

/**
* A test-only ContextManager that uses an in-memory stack to keep track of
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the existing stack context manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would require adding a dependency to opentelemetry-web only for the purpose of running this test. That seemed even more awkward than this. Or perhaps there's some reorganization that can/should happen?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe a follow up is to move it into core as there is nothing inherently web-specific about it. For now, it's fine to keep this here I guess.

Copy link
Member

Choose a reason for hiding this comment

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

you can add this dependency in dev dependency, if there will be any changes to context manager the web will never fail again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moving into core would potentially make sense to me. at the time, all the context managers seemed to be in their usage-specific modules so i hesitated to attempt things like that being new, prior to feedback. up for doing that with this PR or separately. i suppose a separate PR might be better for limiting scope of changes/review?

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 this can be handled in follow up

@obecny
Copy link
Member

obecny commented Jul 24, 2020

is the spec merged and final for that ?

@dyladan
Copy link
Member

dyladan commented Jul 24, 2020

is the spec merged and final for that ?

The spec issue has had no movement and many of the other SIGs have already implemented this type of API.

@dyladan dyladan added the enhancement New feature or request label Aug 12, 2020
@vmarchaud
Copy link
Member

@open-telemetry/javascript-approvers This one is open for 3 weeks now, would be nice to have a couple more reviews in order to move forward so we can remove the header x-opentelemetry-outgoing-request :)

packages/opentelemetry-tracing/test/Tracer.test.ts Outdated Show resolved Hide resolved
packages/opentelemetry-core/test/context/context.test.ts Outdated Show resolved Hide resolved
packages/opentelemetry-core/test/context/context.test.ts Outdated Show resolved Hide resolved
packages/opentelemetry-core/src/context/context.ts Outdated Show resolved Hide resolved
packages/opentelemetry-tracing/src/Tracer.ts Outdated Show resolved Hide resolved
@@ -25,15 +25,16 @@ import { ExportResult } from '@opentelemetry/core';
*/
export class InMemorySpanExporter implements SpanExporter {
private _finishedSpans: ReadableSpan[] = [];
private _stopped = false;
protected _stopped = false;
Copy link
Member

Choose a reason for hiding this comment

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

please add some doc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@obecny what sort of doc are you looking for?

do you want a comment on why this is protected? that seems like something that would quickly get out of date with future usages, outside of the obvious fact it is now available to classes extending the InMemorySpanExporter.

or are you looking for something else?

Copy link
Member

Choose a reason for hiding this comment

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

If something is protected its meaning should be documented so that when someone subclasses this they know what the property represents. It could be argued it is obvious in this case, but it is good practice in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. i don't see that done for any other protected members (non-function) in the code base and somewhat debate the value here but am happy to go along with the standards y'all are looking for.

Copy link
Member

Choose a reason for hiding this comment

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

@michaelgoin I agree we haven't been great at being consistent about this, and in many cases the value is questionable anyways.

@michaelgoin
Copy link
Contributor Author

@obecny should have everything in order now, if you wanna give another scan. thanks!

@dyladan
Copy link
Member

dyladan commented Aug 21, 2020

@obecny should have everything in order now, if you wanna give another scan. thanks!

He is on vacation until monday. Looks like his concerns are addressed, but maybe @mayurkale22 can give you the second maintainer review in his place here.

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.

One minor style suggestion, but otherwise looks good

Comment on lines 122 to 123
const value = context.getValue(SUPPRESS_INSTRUMENTATION_KEY) as boolean;
return !!value;
Copy link
Member

Choose a reason for hiding this comment

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

I would remove the extraneous cast and assignment, but this is minor.

Suggested change
const value = context.getValue(SUPPRESS_INSTRUMENTATION_KEY) as boolean;
return !!value;
return Boolean(context.getValue(SUPPRESS_INSTRUMENTATION_KEY));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was definitely giving that the side-eye while I was restructuring. Thanks for the extra push. :)

Copy link
Member

Choose a reason for hiding this comment

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

It's probably minor, but since this function is likely to be called a lot, I think the performance benefit alone of skipping the assignment is worth it since you don't really lose anything here.

@dyladan
Copy link
Member

dyladan commented Aug 25, 2020

@obecny this is now split into separate methods suppressInstrumentation and unsuppressInstrumentation.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

split looks great now and is straightforward, thx for these changes :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use context to stop tracing
4 participants