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 a status condition to ic for instrumentation details #2408

Merged
merged 16 commits into from
Feb 9, 2025

Conversation

blumamir
Copy link
Collaborator

@blumamir blumamir commented Feb 7, 2025

This adds a condition that describes why the workload is instrumented (which kind of source), along with it's name for better troubleshooting and traceability.

Examples:

image

The function in k8sutils now returns more info about the decision, which can later be used for odigos describe and possibly be also persisted with ic delete if we ever need finalizers for it.

@BenElferink BenElferink added the enhancement New feature or request label Feb 8, 2025
@RonFed
Copy link
Collaborator

RonFed commented Feb 8, 2025

I'm not sure if a condition is the best approach for this.
Since this addition only describes a reason, and does not describe an event in time -
i.e the existence of an instrumentationConfig implies we decided to instrument, so this condition feels redundant.

We could add a dedicated field for it in the status such as creationReason or instrumentationReason - I think that will probably describe it better.

Regarding the reasons for not instrumenting, I think we could add those to the CR if and when we'll implement a finalizer for it. Right now the instrumentationConfig should not exist if we decided not to instrument. Those reasons are great for describe - but not sure they should be part of the API - at least not for now.

@blumamir blumamir merged commit c3c9f2d into odigos-io:main Feb 9, 2025
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants