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

Upgrading Elasticsearch to OTel version 1.0.2 #83

Merged
merged 12 commits into from
Mar 10, 2021
Merged

Upgrading Elasticsearch to OTel version 1.0.2 #83

merged 12 commits into from
Mar 10, 2021

Conversation

ejsmith
Copy link
Contributor

@ejsmith ejsmith commented Feb 25, 2021

This PR upgrades the Elasticsearch instrumentation to OTel version 1.0.2 and serves as an example of how to get rid of ActivitySourceAdapter as well as use the new AddLegacyActivity method.

It also adds a new OpenTelemetry.Contrib.Shared project that contains all of the source shared from the main OTel .NET library that is necessary to create new instrumentations. The library is not meant to be directly referenced. Instead an instrumentation library should use the IncludeSharedInstrumentationSource MSBuild property in order to have the shared source files included in their project.

@ejsmith ejsmith requested a review from a team as a code owner February 25, 2021 23:41
@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #83 (2ff8d0b) into main (fd31e5b) will decrease coverage by 0.54%.
The diff coverage is 70.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
- Coverage   73.16%   72.62%   -0.55%     
==========================================
  Files          41       40       -1     
  Lines        1077     1063      -14     
==========================================
- Hits          788      772      -16     
- Misses        289      291       +2     
Impacted Files Coverage Δ
...ntation/ElasticsearchInstrumentationEventSource.cs 0.00% <0.00%> (ø)
.../ElasticsearchRequestPipelineDiagnosticListener.cs 82.35% <76.66%> (-2.06%) ⬇️
...lasticsearch/ElasticsearchClientInstrumentation.cs 100.00% <100.00%> (+12.50%) ⬆️
...earch/ElasticsearchClientInstrumentationOptions.cs 100.00% <100.00%> (ø)
...n.Elasticsearch/TracerProviderBuilderExtensions.cs 87.50% <100.00%> (+4.16%) ⬆️

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

changes to move away from ActivitySourceAdapater is good and the tests confirm its working.

@cijothomas
Copy link
Member

@ejsmith could you update the PR title and description to match the current state.
Changes look good to me, and can be merged once the CI is green.

@ejsmith
Copy link
Contributor Author

ejsmith commented Mar 2, 2021

@cijothomas I'm still waiting for resolution on open-telemetry/opentelemetry-dotnet#1856 because I need access to SuppressInstrumentationScope.IncrementIfTriggered and SuppressInstrumentationScope.DecrementIfTriggered inside of the DiagnosticSourceListener class in order to make nested activity suppression work correctly.

@ejsmith ejsmith changed the title Upgrading Elasticsearch to OTel version to 1.0.1 Upgrading Elasticsearch to OTel version 1.0.2 Mar 2, 2021
@ejsmith
Copy link
Contributor Author

ejsmith commented Mar 2, 2021

@cijothomas is the title and description better now? All the checks should be passing now as well.

@alexvaluyskiy
Copy link
Contributor

I’ve updated the instrumentation for MassTransit and EntityFrameworkCore. But I have to add the same shared files, like in this PR.
@ejsmith @cijothomas Is there any chance this PR will be merged soon? Or I should not wait and create my own PR?

@ejsmith
Copy link
Contributor Author

ejsmith commented Mar 9, 2021

Still waiting for resolution on open-telemetry/opentelemetry-dotnet#1856 before this can be merged. I'm attempting to wait patiently. 😆

@cijothomas
Copy link
Member

Thanks for waiting!
We'll close on the dependent PRs today, and unblock all the PRs in this repo :)

@cijothomas
Copy link
Member

Adapt changes like this open-telemetry/opentelemetry-dotnet#1893 and this should unblock this PR including the supressdownstream functionality.

@ejsmith
Copy link
Contributor Author

ejsmith commented Mar 10, 2021

@cijothomas done

@cijothomas cijothomas closed this Mar 10, 2021
@cijothomas cijothomas reopened this Mar 10, 2021
@cijothomas cijothomas merged commit 8286895 into open-telemetry:main Mar 10, 2021
@ejsmith ejsmith mentioned this pull request Mar 10, 2021
@ejsmith ejsmith deleted the main branch May 6, 2021 18:56
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.

5 participants