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

Instrumentation libraries improvements #3495

Closed
vishweshbankwar opened this issue Jul 26, 2022 · 6 comments · Fixed by #3604
Closed

Instrumentation libraries improvements #3495

vishweshbankwar opened this issue Jul 26, 2022 · 6 comments · Fixed by #3604
Labels
enhancement New feature or request

Comments

@vishweshbankwar
Copy link
Member

vishweshbankwar commented Jul 26, 2022

Instrumentation libraries improvements

Subscribing to DiagnosticSource events

Instrumentation libraries subscribe to the diagnostic source events by looking at event names
that ends with Start, Stop, Exception etc ref. This could result in subscribing to events which we did not
intend to. One such example is #3475.

Discussion: Should we match with explicit names rather than pattern matching?

Relying on Activity.Current

When the diagnostic source events are written by the respective framework libraries. Usually the activity is
not part of the payload or activity itself is not started by the framework e.g. SQL instrumentation. In order
to enrich/modify activities instrumentation libraries rely on Activity.Current ref which may cause issues e.g. #3490.

Action : Review any more cases like the one shown in example.

Documentation

We do not have a detailed inventory of events that the instrumentation libraries in this
repo subscribes to. Having this documented for each instrumentation would help provide more clarity to users. Also, this would help us analyze and make decision on whether we need to support more events or update any existing ones.

Action: Update Readme documents for each instrumentation library with the event names it subscribes to.

Avoid reflection (ASP.NET Core)

Reflection is used to extract properties from events payload (e.g.). This was done in order to avoid nuget reference. Now that we have FrameworkReference Microsoft.AspNetCore.App, could we avoid reflection?

@vishweshbankwar vishweshbankwar added the enhancement New feature or request label Jul 26, 2022
@vishweshbankwar
Copy link
Member Author

@cijothomas @alanwest - let me know if I missed something or need to add more details.

@cijothomas
Copy link
Member

Linked issue : #3373

@Kielek
Copy link
Contributor

Kielek commented Aug 3, 2022

@vishweshbankwar, on the AutoInstrumentation side also for DevOps scenarios it will be great to have possibility to reconfigure all instrumentation libraries by the environmental variables.

Please check open-telemetry/opentelemetry-dotnet-contrib#519

@vishweshbankwar
Copy link
Member Author

@vishweshbankwar, on the AutoInstrumentation side also for DevOps scenarios it will be great to have possibility to reconfigure all instrumentation libraries by the environmental variables.

Please check open-telemetry/opentelemetry-dotnet-contrib#519

@Kielek - Could you please open a new issue to track this which can be used for discussion as well.

@Kielek
Copy link
Contributor

Kielek commented Oct 24, 2022

@vishweshbankwar, as I know @CodeBlanch is doing a lot of changes to make as closer to this goal.

@CodeBlanch, do you need a separate task for now?

@vishweshbankwar
Copy link
Member Author

Marking this complete. All the PRs related to this are linked above.

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
4 participants