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

feat: add otlp endpoint config and cli flag #994

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

fabriziosestito
Copy link
Contributor

@fabriziosestito fabriziosestito commented Dec 5, 2024

Description

This PR adds a new --otlp-endpoint flag and the OTEL_EXPORTER_OTLP_ENDPOINT environment variable, allowing users to specify a custom gRPC OTLP endpoint for exporting both metrics and traces.

Part of #993

Test

Updates existing integration tests.

@fabriziosestito fabriziosestito self-assigned this Dec 5, 2024
@fabriziosestito fabriziosestito requested a review from a team as a code owner December 5, 2024 12:15
@fabriziosestito fabriziosestito added this to the 1.20 milestone Dec 5, 2024
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 62.64%. Comparing base (8bb10f8) to head (5b843fb).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/main.rs 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #994      +/-   ##
==========================================
- Coverage   62.71%   62.64%   -0.08%     
==========================================
  Files          17       17              
  Lines        1046     1052       +6     
==========================================
+ Hits          656      659       +3     
- Misses        390      393       +3     
Flag Coverage Δ
integration-tests 55.18% <66.66%> (-0.03%) ⬇️
unit-tests 38.09% <0.00%> (-0.12%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jvanz jvanz left a comment

Choose a reason for hiding this comment

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

Unless we really want a CLI flag for this, I think we do not need it. The rust library does check the environment variable for us.

I would add a CLI flag to define the certificate now instead. ;)

@fabriziosestito
Copy link
Contributor Author

fabriziosestito commented Dec 5, 2024

@jvanz I think we should keep both the CLI flag and the config entry. It’s better to have consistent configurations, and exposing the certificate path only through the CLI could be confusing. Having the config struct map the otlp CLI values also makes integration tests easier.

@fabriziosestito fabriziosestito force-pushed the feat/add-otlp-endpoint-flag branch from 896e93d to 5b843fb Compare December 5, 2024 14:45
@jvanz
Copy link
Member

jvanz commented Dec 5, 2024

@jvanz I think we should keep both the CLI flag and the config entry. It’s better to have consistent configurations, and exposing the certificate path only through the CLI could be confusing. Having the config struct map the otlp CLI values also makes integration tests easier.

Fair enough. I'm fine with that. I was mentioning that just to ensure that you're aware of this possibility. ;)

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

LGTM

@flavio flavio merged commit 0fbfec9 into kubewarden:main Dec 6, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants