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

Golang instrumentation featuregates into cli #2750

Merged
merged 10 commits into from
Apr 26, 2024

Conversation

yuriolisa
Copy link
Contributor

Description:

Changed featuregates.autoinstrumentation.go to cli.
Link to tracking Issue(s):

Testing:

Documentation:

Signed-off-by: Yuri Sa <yurimsa@gmail.com>
Signed-off-by: Yuri Sa <yurimsa@gmail.com>
Signed-off-by: Yuri Sa <yurimsa@gmail.com>
@yuriolisa yuriolisa requested a review from a team as a code owner March 11, 2024 21:21
main.go Outdated Show resolved Hide resolved
Signed-off-by: Yuri Sa <yurimsa@gmail.com>
@yuriolisa yuriolisa changed the title Featuregates into cli Golang instrumentation featuregates into cli Mar 13, 2024
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

LGTM.

the e2e tests are failing, if you re-push it should pass

component: 'operator'

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Change featuregates auto-instrumentation.go to CLI
Copy link
Member

Choose a reason for hiding this comment

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

we should document exactly which flag is changing and what is the new flag.

@swiatekm-sumo
Copy link
Contributor

The reason these tests fail is that this feature gate is enabled in the default manifests. #1555 added this, and I'm not entirely sure why. @TylerHelmuth do you recall?

@TylerHelmuth
Copy link
Member

@swiatekm-sumo back then there was no other way (that I knew of at least) to enable the feature for the tests. Specific discussion here: #1555 (comment)

@swiatekm-sumo
Copy link
Contributor

@swiatekm-sumo back then there was no other way (that I knew of at least) to enable the feature for the tests. Specific discussion here: #1555 (comment)

Is there actually any difference between enabling the flag by default in the code vs doing so in the manifests? I want to understand if this instrumentation should be enabled or disabled by default. Right now, we have the ability to add operator flags for specific E2E test suites.

@TylerHelmuth
Copy link
Member

Right now, we have the ability to add operator flags for specific E2E test suites.

Sweet, we should enable it for the tests that need it, but I think disabled by default is still correct since it is alpha.

@TylerHelmuth
Copy link
Member

Clicked the wrong button

@swiatekm-sumo
Copy link
Contributor

@yuriolisa it sounds like you need to add something the following:

.PHONY: prepare-e2e-with-operator-flags
prepare-e2e-with-operator-flags: OPERATOR_ARG = $(OPERATOR_FLAGS)
prepare-e2e-with-operator-flags chainsaw  add-operator-arg prepare-e2e

And then use it in the CI for setup of instrumentation tests.

Signed-off-by: Yuri Sa <yurimsa@gmail.com>
@pavolloffay pavolloffay merged commit 16e1134 into open-telemetry:main Apr 26, 2024
31 checks passed
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed e2e job

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

---------

Signed-off-by: Yuri Sa <yurimsa@gmail.com>
rubenvp8510 pushed a commit to rubenvp8510/opentelemetry-operator that referenced this pull request May 7, 2024
* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed featuregate into CLI - instrumentation go

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Changed e2e job

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

---------

Signed-off-by: Yuri Sa <yurimsa@gmail.com>
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.go
4 participants