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

Change nginx instrumentation into command line #2777

Conversation

dexter0195
Copy link
Contributor

Description:
Change nginx instrumentation into command line

Link to tracking Issue(s):
#2676

Testing:

Documentation:

@dexter0195 dexter0195 requested a review from a team March 21, 2024 22:22
@dexter0195
Copy link
Contributor Author

Hey 😃
Can you help me understand why the e2e are all failing at the step prepare-e2e please 🙏 ?

@TylerHelmuth
Copy link
Member

The instrumentation was off by default, so you'll need to update some tests to re-enable it. You can look at #2750 as an example.

main.go Outdated Show resolved Hide resolved
@dexter0195
Copy link
Contributor Author

The instrumentation was off by default, so you'll need to update some tests to re-enable it. You can look at #2750 as an example.

Hey @TylerHelmuth thank you for the advice, I had a look at the pull request you said and performed the required changes.

However the tests were still failing so I found that we were activating the feature gate in config/manager/manager.yaml , which after its removal was no longer valid. So I removed it and no the e2e tests are passing ✅

Please tell me if the solution I found was the right one or if there is something else I should do 🙏
Thank you

@swiatekm
Copy link
Contributor

It looks like we have both the Go and the Nginx autoinstrumentations enabled in our release manifests, I believe unintentionally? If I recall correctly, the intent was to just enable them in E2E tests, right @TylerHelmuth ? In that case, we should probably just accept this state of affairs and set the flags themselves to be enabled by default. WDYT @jaronoff97 @pavolloffay ?

To be clear, I think this PR can be merged as-is, it's not changing the current behaviour.

@TylerHelmuth
Copy link
Member

If I recall correctly, the intent was to just enable them in E2E tests

Correct.

@dexter0195
Copy link
Contributor Author

Hey 😃
thank you for the review and your comments ! Do you think we can merge this ?

component: operator

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: change nginx instrumentation feature gate into command line flag --enable-nginx-instrumentation
Copy link
Member

Choose a reason for hiding this comment

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

Please mention here what is the feature gate flag that is being changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ✅

@jaronoff97 jaronoff97 merged commit d4b24f3 into open-telemetry:main Apr 1, 2024
31 checks passed
@dexter0195 dexter0195 deleted the feat/autoinstrumentation-nginx-cli-flags branch April 1, 2024 16:43
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* feat: change nginx instrumentation to cli flag

* set nginx instrumentation to default false

* update config/manager

* enable nginx in config/manager

* update chloggen
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.

Change instrumentation feature gates into normal command-line flags: operator.autoinstrumentation.nginx
5 participants