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

Merge aspnet-telemetrycorrelation branch back to main #2233

Merged
merged 18 commits into from
Aug 11, 2021

Conversation

reyang
Copy link
Member

@reyang reyang commented Aug 4, 2021

Part of #2222.

Note: we need to merge this PR by keeping all the commits rather than squashing them (so we know what's been copied over directly vs. changed by the OpenTelemetry .NET community).
Note: I didn't include the changelog yet, we might want to add a single entry with all the related PRs, similar like what we've done in the spec repo https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md#metrics-2.

@CodeBlanch I think we have two options, want to get your opinion:

  1. merge this to main right now, and you will continue the work in main branch (with the benefit of CI, less chance of hitting merge conflict when we touch the ASP.NET instrumentation lib, etc.)
  2. keep this in the aspnet-telemetrycorrelation branch, which means you will continue the refactor (including renaming the package/namespace and the integration with ASP.NET instrumentation), and once you finished, merge this PR (which will be bigger at that time) to main.

@reyang reyang closed this Aug 4, 2021
@reyang reyang reopened this Aug 4, 2021
@reyang reyang changed the title [WIP] Test [WIP] Merge aspnet-telemetrycorrelation branch back to main Aug 4, 2021
@reyang reyang closed this Aug 4, 2021
@reyang reyang reopened this Aug 4, 2021
@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #2233 (ddaa5ab) into main (24226a5) will decrease coverage by 1.03%.
The diff coverage is 54.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2233      +/-   ##
==========================================
- Coverage   80.14%   79.11%   -1.04%     
==========================================
  Files         227      237      +10     
  Lines        7173     7484     +311     
==========================================
+ Hits         5749     5921     +172     
- Misses       1424     1563     +139     
Impacted Files Coverage Δ
...ry.Instrumentation.AspNet/AspNetInstrumentation.cs 100.00% <ø> (ø)
....AspNet.TelemetryHttpModule/TelemetryHttpModule.cs 2.27% <2.27%> (ø)
....TelemetryHttpModule/AspNetTelemetryEventSource.cs 26.92% <26.92%> (ø)
...et.TelemetryHttpModule/Internal/HeaderUtilities.cs 33.33% <33.33%> (ø)
...Net.TelemetryHttpModule/Internal/HttpRuleParser.cs 46.42% <46.42%> (ø)
...t.TelemetryHttpModule/Internal/BaseHeaderParser.cs 72.72% <72.72%> (ø)
...n.AspNet.TelemetryHttpModule/ActivityExtensions.cs 78.78% <78.78%> (ø)
...ation.AspNet.TelemetryHttpModule/ActivityHelper.cs 80.00% <80.00%> (ø)
...lemetryHttpModule/Internal/NameValueHeaderValue.cs 88.09% <88.09%> (ø)
...elemetryHttpModule/Internal/GenericHeaderParser.cs 100.00% <100.00%> (ø)
... and 13 more

@reyang reyang changed the title [WIP] Merge aspnet-telemetrycorrelation branch back to main Merge aspnet-telemetrycorrelation branch back to main Aug 5, 2021
@reyang reyang marked this pull request as ready for review August 5, 2021 01:15
@reyang reyang requested a review from a team August 5, 2021 01:15
@cijothomas
Copy link
Member

marking as "donotmerge" until we temporarily enable merge-with-keep-commits.

@reyang
Copy link
Member Author

reyang commented Aug 5, 2021

marking as "donotmerge" until we temporarily enable merge-with-keep-commits.

And we want to wait to get approval (or rejection) from @CodeBlanch as he will be taking it further after I finished the grunt work.

@CodeBlanch
Copy link
Member

I think we're good to merge this now. @cijothomas What do we do with API Compatability check on new projects?

@cijothomas
Copy link
Member

I think we're good to merge this now. @cijothomas What do we do with API Compatability check on new projects?

Similar to the below:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.Prometheus/OpenTelemetry.Exporter.Prometheus.csproj#L14-L18

@reyang reyang closed this Aug 10, 2021
@reyang reyang reopened this Aug 10, 2021
@reyang
Copy link
Member Author

reyang commented Aug 11, 2021

I think we're good to merge this now. @cijothomas What do we do with API Compatability check on new projects?

Similar to the below:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.Prometheus/OpenTelemetry.Exporter.Prometheus.csproj#L14-L18

I've disabled the API Compat check for this module, also fixed a trailing space issue in the README doc.
This PR should be good to merge now.

@cijothomas cijothomas merged commit c9c7869 into main Aug 11, 2021
@cijothomas cijothomas deleted the aspnet-telemetrycorrelation branch August 11, 2021 03:50
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.

3 participants