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

[repo] Add dedicated CI for AspNet projects #1386

Merged
merged 5 commits into from
Oct 11, 2023

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Oct 4, 2023

Changes

  • Adds dedicated CI for AspNet projects.
  • Switches to a single tag for AspNet & AspNet.TelemetryHttpModule projects so that they will always be released together.
  • Modifies Component.Package.yml to support multiple packages being released from the same tag.

Details

Why switch to a single tag?

  • If AspNet.TelemetryHttpModule is being changed, we always want to also push AspNet.

  • If AspNet is being changed, we could run into trouble without also pushing AspNet.TelemetryHttpModule. For something like a pure bug fix, it would be fine to only push AspNet. However, AspNet gets its OTel API and DS references through the HttpModule project. It also directly depends on the exposed AspNet.TelemetryHttpModule APIs. If AspNet.TelemetryHttpModule was changed, for example do to an OTel API version bump, the AspNet package will be compiled against those bits but packaged targeting whatever the last tag was (older bits). Users consuming that package could run into big issues. Pushing the two packages together makes it always safe without having to worry about what may be lurking in the repo.

@CodeBlanch CodeBlanch added comp:instrumentation.aspnet.telemetryhttpmodule Things related to OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet labels Oct 4, 2023
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #1386 (8cb8f16) into main (71655ce) will decrease coverage by 0.16%.
Report is 21 commits behind head on main.
The diff coverage is 76.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1386      +/-   ##
==========================================
- Coverage   73.91%   73.76%   -0.16%     
==========================================
  Files         267      258       -9     
  Lines        9615     9551      -64     
==========================================
- Hits         7107     7045      -62     
+ Misses       2508     2506       -2     
Flag Coverage Δ
unittests-Solution 80.38% <41.17%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...orter.Geneva/MsgPackExporter/MsgPackLogExporter.cs 95.10% <100.00%> (+1.76%) ⬆️
...searchClient/ElasticsearchClientInstrumentation.cs 100.00% <100.00%> (ø)
...ityFrameworkCore/EntityFrameworkInstrumentation.cs 100.00% <100.00%> (ø)
...ation/EntityFrameworkInstrumentationEventSource.cs 12.00% <100.00%> (ø)
...ation.Owin/Implementation/DiagnosticsMiddleware.cs 89.42% <100.00%> (-2.44%) ⬇️
....Owin/Implementation/OwinInstrumentationMetrics.cs 100.00% <100.00%> (ø)
...inInstrumentationMeterProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
...Instrumentation.Quartz/QuartzJobInstrumentation.cs 83.33% <100.00%> (+1.51%) ⬆️
...entation.Wcf/Implementation/InstrumentedChannel.cs 100.00% <100.00%> (ø)
...f/Implementation/InstrumentedChannelFactoryBase.cs 100.00% <100.00%> (ø)
... and 16 more

... and 33 files with indirect coverage changes

@CodeBlanch CodeBlanch marked this pull request as ready for review October 4, 2023 05:48
@CodeBlanch CodeBlanch requested a review from a team October 4, 2023 05:48
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

Previously, we have had technically two release on the GH side, now we will have only one with link to the AspNet project. Changelog for TelemetryModule will be not linked.

Ref: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/releases/tag/Instrumentation.AspNet.TelemetryHttpModule-1.0.0-rc9.9 and https://github.com/open-telemetry/opentelemetry-dotnet-contrib/releases/tag/Instrumentation.AspNet-1.0.0-rc9.9

It will be great to have two of them in on GH release. I think it can be done in follow up PR.

@CodeBlanch CodeBlanch marked this pull request as draft October 5, 2023 00:31
@CodeBlanch
Copy link
Member Author

Good call @Kielek! I'll look into it a bit.

@CodeBlanch CodeBlanch marked this pull request as ready for review October 8, 2023 05:44
@CodeBlanch
Copy link
Member Author

@Kielek I just pushed support for including multiple packages in the GitHub release created by the package workflow.

@CodeBlanch
Copy link
Member Author

I realized there is one more issue with this. Here...

- name: dotnet test test/${{ inputs.project-name }}.Tests
run: dotnet test test/${{ inputs.project-name }}.Tests --configuration Release --no-restore --no-build

...we only test the AspNet project. I need to switch it to test all projects (HttpModule too). To do that I have to tweak all the .proj files I have added for all the component-specific CIs to have a test target. Thinking I'm going to merge this and do that as a follow-up to keep the diff manageable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.aspnet.telemetryhttpmodule Things related to OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants