-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add imports to instrumentations example in browser usage docs #5710
base: main
Are you sure you want to change the base?
Conversation
The committers listed above are authorized under a signed CLA. |
Thanks, @gilisho! @open-telemetry/javascript-approvers, PTAL. |
registerInstrumentations({ | ||
instrumentations: [ | ||
new DocumentLoadInstrumentation(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding this was not agreed on over at #4951, right?
const provider = new WebTracerProvider({ | ||
spanProcessors: [new SimpleSpanProcessor(new ConsoleSpanExporter())], | ||
}); | ||
|
||
provider.register({ | ||
// Changing default contextManager to use ZoneContextManager - supports asynchronous operations - optional | ||
contextManager: new ZoneContextManager(), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is already explained above, should we duplicate the setup?
import { | ||
ConsoleSpanExporter, | ||
SimpleSpanProcessor, | ||
} from '@opentelemetry/sdk-trace-base'; | ||
import { WebTracerProvider } from '@opentelemetry/sdk-trace-web'; | ||
import { DocumentLoadInstrumentation } from '@opentelemetry/instrumentation-document-load'; | ||
import { ZoneContextManager } from '@opentelemetry/context-zone'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't do the duplicate setup then this can also be removed.
@gilisho can you take a look at the provided feedback? TY |
Fixes #4951.
This PR adds missing imports in docs example. As a OTEL beginner, the instrumentation libraries that should be installed are not trivial. I've added these imports as a small improvement to the docs.