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: add span processor #136

Merged
merged 1 commit into from
Aug 22, 2019

Conversation

mayurkale22
Copy link
Member

Idea: The SpanProcessor interface allows users to build frameworks like exporter/z-pages/etc. The SpanProcessor interface should allow intercepting Spans immediately after start and immediately after end.

OpenCensus contains the same kind of interface:
https://github.com/census-instrumentation/opencensus-node/blob/056523d83d21449e3bd888fec624d33c129951cc/packages/opencensus-core/src/trace/model/types.ts#L261-L266

This exist in java but aren't yet included in the spec.

@codecov-io
Copy link

codecov-io commented Jul 26, 2019

Codecov Report

Merging #136 into master will decrease coverage by 2.61%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
- Coverage   94.31%   91.69%   -2.62%     
==========================================
  Files          47       48       +1     
  Lines        1777     1349     -428     
  Branches      117       94      -23     
==========================================
- Hits         1676     1237     -439     
- Misses        101      112      +11
Impacted Files Coverage Δ
...ages/opentelemetry-basic-tracer/src/BasicTracer.ts 81.81% <0%> (-16.33%) ⬇️
packages/opentelemetry-basic-tracer/src/Span.ts 85.71% <0%> (-14.29%) ⬇️
...ry-scope-async-hooks/src/AsyncHooksScopeManager.ts 95% <0%> (-2.68%) ⬇️
...kages/opentelemetry-basic-tracer/test/Span.test.ts 100% <0%> (ø) ⬆️
...metry-node-tracer/src/instrumentation/constants.ts 100% <0%> (ø) ⬆️
.../opentelemetry-node-tracer/test/NodeTracer.test.ts 0% <0%> (ø) ⬆️
.../opentelemetry-core/src/trace/spancontext-utils.ts 100% <0%> (ø) ⬆️
...s/opentelemetry-core/test/trace/NoopTracer.test.ts 100% <0%> (ø) ⬆️
...try-node-tracer/test/instrumentation/utils.test.ts 100% <0%> (ø) ⬆️
...entelemetry-core/test/trace/TracerDelegate.test.ts 100% <0%> (ø) ⬆️
... and 15 more

Copy link
Member

@rochdev rochdev left a comment

Choose a reason for hiding this comment

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

I don't think this is flexible enough. Can every method call be hooked into instead? Also, maybe event emitters are better suited for this use case?

@draffensperger
Copy link
Contributor

I'd rather avoid event emitters if possible since I think they are Node-specific. @rochdev what are the use cases you have in mind for hooking into the other methods of Span? Could that be achieved by having a different tracer that returns a subclassed Span implementation?

I guess my thinking is that if the most common cases involve exporters that would operate on a span when it ends, then the extra code (JS bytes for the browser) and complexity of arbitrary hooks might not be needed in the basic Span implementation.

@bg451
Copy link
Member

bg451 commented Jul 29, 2019

👍 This seems like a good start.

Maybe the wrong PR to discuss this on, but one of the main benefits of having hooks/event emitters for every span method is you can then stream to a sidecar rather than going through an exporter, eliminating all buffering and most span state in the application (you'd need to maintain the span context but that's it). An implementation like that though would probably replace the SDK.

@rochdev I'm also curious, are there any hook/event emitter examples you have in mind?

Copy link
Contributor

@danielkhan danielkhan left a comment

Choose a reason for hiding this comment

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

I like the idea. Lifecycle hooks make life much easier and this is a good start.

@draffensperger
Copy link
Contributor

Per the SIG: we agreed that lifecycle hooks are valuable and should be implemented with a simple callback-based mechanism.

@rochdev
Copy link
Member

rochdev commented Aug 1, 2019

To give a bit more context, at Datadog we have many features outside of the capability of OpenTelemetry, and we need to be able to call specific methods that are vendor-specific at different times during the lifecycle of the tracer. This is also not limited to Span, as we need such hooks also for the scope manager for example.

By having hooks for every function available on every class of the tracer, it would allow us to support OpenTelemetry without requiring the user to choose between using OpenTelemetry with less features or not using it to get more features.

@mayurkale22 mayurkale22 merged commit b208b2b into open-telemetry:master Aug 22, 2019
@mayurkale22 mayurkale22 deleted the span_processor branch August 22, 2019 18:39
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
…dis-4.x

chore(deps): update dependency @types/ioredis to v4.17.2
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.

6 participants