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

using opentelemetry.io* namespace for extracing resource based on annotations from k8s Api #2330 #3010

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Horiodino
Copy link

@Horiodino Horiodino commented Jun 4, 2024

change_type: sdk.go

  • Modified the logic for iterating over pod annotations, utilizing pod.GetAnnotations() to identify and extract resources using the specified namespace prefix (constants.ReservedNamespace).

  • Implemented the use of the OpenTelemetry.io namespace for extracting resource annotations from Kubernetes pods.

  • Enhanced handling to streamline the extraction process and avoid overwriting existing resource mappings.

issues: [https://github.com//issues/2181]

#2330

TestCase ✅

?       github.com/open-telemetry/opentelemetry-operator/pkg/constants  [no test files]
ok      github.com/open-telemetry/opentelemetry-operator/pkg/collector/upgrade  
ok      github.com/open-telemetry/opentelemetry-operator/pkg/featuregate        
ok      github.com/open-telemetry/opentelemetry-operator/pkg/instrumentation    
ok      github.com/open-telemetry/opentelemetry-operator/pkg/instrumentation/upgrade    
ok      github.com/open-telemetry/opentelemetry-operator/pkg/sidecar    ```

Signed-off-by: Horiodino <holiodin@gmail.com>
@swiatekm
Copy link
Contributor

swiatekm commented Jun 4, 2024

The title of your PRs indicates it's adding support for attribute labels as well resource ones, but the code only seems to implement resources. Are you planning on a follow-up?

@Horiodino
Copy link
Author

oh sorry for that , i have just added anotation for now because annotations are for attaching non-identifying metadata.
well @jaronoff97 should i add labels too ?

@jaronoff97
Copy link
Contributor

I think we should only do from annotations and not labels. @Horiodino can you add a changelog and update the title to specify only annotations?

@Horiodino Horiodino changed the title using opentelemetry.io* namespace for extracing resource and attribute labels from k8s Api #2330 using opentelemetry.io* namespace for extracing resource based on annotations from k8s Api #2330 Jun 6, 2024
@Horiodino
Copy link
Author

done

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

LGTM, small nitpick about naming. Please add a changelog entry as well, you can run make chlog-new to have the tooling generate a template for you.

pkg/constants/env.go Outdated Show resolved Hide resolved
Signed-off-by: Horiodino <holiodin@gmail.com>
@jaronoff97
Copy link
Contributor

@Horiodino last thing to do here is add a changelog via make chlog-new. That will generate a new file in the .chloggen/ You can then edit that file with the necessary details at which point we can merge this :D

@Horiodino
Copy link
Author

@jaronoff97 hearding about changelog first time ! i added it now

.chloggen/extract-resources-ns.yaml Show resolved Hide resolved
.chloggen/extract-resources-ns.yaml Outdated Show resolved Hide resolved
.chloggen/extract-resources-ns.yaml Show resolved Hide resolved
Signed-off-by: Horiodino <holiodin@gmail.com>
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.

Can we please document this in the root readme?

@jaronoff97
Copy link
Contributor

@Horiodino when you get a chance, if you could add some documentation into the README.md then we should be able to merge this! 🙇

…urce

Signed-off-by: Horiodino <holiodin@gmail.com>
Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

Suggested some phrasing changes to the documentation.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Horiodino and others added 2 commits June 26, 2024 18:19
Co-authored-by: Mikołaj Świątek <mail@mikolajswiatek.com>
Co-authored-by: Mikołaj Świątek <mail@mikolajswiatek.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.

None yet

4 participants