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

This library is missing from Open Telemetry registry #7

Closed
jtmalinowski opened this issue Jul 5, 2021 · 7 comments
Closed

This library is missing from Open Telemetry registry #7

jtmalinowski opened this issue Jul 5, 2021 · 7 comments

Comments

@jtmalinowski
Copy link

jtmalinowski commented Jul 5, 2021

Hi @MetinSeylan,
I'm opening this issue just to let you know that I'm adding this library to the registry in this PR: open-telemetry/opentelemetry.io#626. If you'd prefer that Nestjs-OpenTelemetry wasn't in the registry, let me know, otherwise thank you for taking the time to create this library.

@jtmalinowski
Copy link
Author

jtmalinowski commented Jul 6, 2021

I have a couple thoughts after reviewing the code:

  • I think this library would benefit from clearly dividing the instrumentation part (i.e. creating spans, context management, trace id injection in logs) and Nest helpers (services, initialization, etc.)
  • in other words, that division would best be done by exporting a NestInstrumentation, which you can use when initialising Open Telemetry
  • it's not immediately clear if spans will be created for actions/handlers without @Span annotation, it looks to me like they won't, which is a bit unusual for an instrumentation of this kind
  • the purpose of TraceInterceptor is a bit unclear, did OTel propagators not work for you otherwise?

If you think you have the right Nest expertise to drive the design on Nest's end of this library, I can check if @splunk would commit some resources to make this library play a bit more nicely with Open Telemetry - good support for instrumentation in Nest is on our list. Let me know what you think.

@mentos1386
Copy link

@jtmalinowski i think you might be interested in https://github.com/pragmaticivan/nestjs-otel.

@jtmalinowski
Copy link
Author

@mentos1386 thanks for the tip. The repo you linked only provides metrics and some manual instrumentation helpers. Once metrics specification matures, it will be nice to incorporate it.

@jtmalinowski
Copy link
Author

@MetinSeylan I'm closing for now, this library is now in the registry. Maybe there'll be a potential for some more collaboration in the future.

@MetinSeylan
Copy link
Owner

@jtmalinowski @mentos1386 we are using this library as a internal tool in my company, that's why we are decided refactor this library dramatically, we are working on it with my team, we will release new version soon, btw thanks for feedback it's important for us

@jtmalinowski
Copy link
Author

Gotcha.

@MetinSeylan
Copy link
Owner

finally refactored :)

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

No branches or pull requests

3 participants