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

Add EventCounters to AotCompatibility.Test #1322

Merged
merged 4 commits into from
Aug 25, 2023

Conversation

eerhardt
Copy link
Contributor

This ensures that the OpenTelemetry.Instrumentation.EventCounters library stays AOT-compatible.

Contributes to open-telemetry/opentelemetry-dotnet#3429

cc @Yun-Ting

This ensures that the OpenTelemetry.Instrumentation.EventCounters library stays AOT-compatible.

Contributes to open-telemetry/opentelemetry-dotnet#3429
@eerhardt eerhardt requested a review from a team August 25, 2023 01:19
@utpilla utpilla added the comp:instrumentation.eventcounters Things related to OpenTelemetry.Instrumentation.EventCounters label Aug 25, 2023
@utpilla
Copy link
Contributor

utpilla commented Aug 25, 2023

@eerhardt The AOT workflow does not always run. You would have to change the path here: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/.github/workflows/ci-aot.yml#L6-L8 and also just test the AOT compatibility outside of the workflow to ensure that EventCounters instrumentation is actually AOT-compliant.

@Yun-Ting
Copy link
Contributor

@eerhardt, please take this commit: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1324/commits
(I can't push to your branch.)
@utpilla: thanks for reviewing! OpenTelemetry.Instrumentation.EventCounters is currently AOT-compliant. See: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/actions/runs/5978918066/job/16221944116?pr=1324

Copy link
Contributor

@Yun-Ting Yun-Ting left a comment

Choose a reason for hiding this comment

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

LGTM assuming GitHub workflow file got updated. Thanks!

@eerhardt
Copy link
Contributor Author

@eerhardt The AOT workflow does not always run. You would have to change the path here: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/.github/workflows/ci-aot.yml#L6-L8 and also just test the AOT compatibility outside of the workflow to ensure that EventCounters instrumentation is actually AOT-compliant.

I've tested it locally, and it worked. Is there a way to manually kick off the CI leg on this change?

@Yun-Ting's Draft PR had these same changes, and the AOT leg passed on it: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/actions/runs/5978918066/job/16221944116

@utpilla
Copy link
Contributor

utpilla commented Aug 25, 2023

Is there a way to manually kick off the CI leg on this change?

@eerhardt You could add a simple workflow_dispatch event to the workflow and then you should be able to manually run the CI on your fork: https://docs.github.com/en/actions/using-workflows/manually-running-a-workflow.

@eerhardt
Copy link
Contributor Author

Thanks for the help! I was able to successfully run the workflow in my fork:

https://github.com/eerhardt/opentelemetry-dotnet-contrib/actions/runs/5979901435/job/16224947116

@utpilla utpilla merged commit 34b6323 into open-telemetry:main Aug 25, 2023
@eerhardt eerhardt deleted the AddEventCountersAot branch August 28, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.eventcounters Things related to OpenTelemetry.Instrumentation.EventCounters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants