Add reported name field to Source, deprecate the odigos.io/reported-name annotation #2298
Conversation
| // ReportedName determines "service.name" resource attribute which will be reported by the instrumentations of this source. | ||
| // If not set, the workload name will be used. | ||
| // It is not valid for namespace sources. | ||
| ReportedName string `json:"reportedName,omitempty"` |
There was a problem hiding this comment.
needs // +kubebuilder:validation:Optional (generated docs show it as required)
There was a problem hiding this comment.
It looks like we have Required for all of our fields and // +kubebuilder:validation:Optional is not doing anything.
Adding // +optional did the trick, which seems to be what the genref looks for
I can do a follow up PR adding it to all our fields. (and maybe remove // +kubebuilder:validation:Optional which seems to not do anything?)
There was a problem hiding this comment.
ah, weird. I think that's a bug in the genref, they should definitely be parsing +kubebuilder tags too
A follow up to add // +optional would be good. I think it will only affect the docs, because the controller-gen scripts should be respecting either tag. Meaning, I think our CRDs are accurate
| // any more changes to the annotation will not be reflected in the source. | ||
| // This is valid, since the annotation is deprecated and we want to encourage users to use Source CR. | ||
| if kind != workload.WorkloadKindNamespace && source.Spec.ReportedName == "" { | ||
| source.Spec.ReportedName = reportedName |
There was a problem hiding this comment.
could this update the Source even if one is already set? So someone could update the label and the migration updates the Source for them.
| allErrs = append(allErrs, field.Invalid( | ||
| field.NewPath("spec").Child("reportedName"), | ||
| source.Spec.ReportedName, | ||
| "Namespace Source must not have a reportedName, to configure a reported name for a workload edit its Source", |
There was a problem hiding this comment.
| "Namespace Source must not have a reportedName, to configure a reported name for a workload edit its Source", | |
| "Reported name is not valid for Namespace sources, only valid for Workload Sources", |
just a little clearer imo
| # Using the deprecated way of instrumenting a namespace, | ||
| # after v1.0.144, use ../../common/apply/instrument-default-ns.yaml | ||
| - script: | ||
| content: kubectl label namespace default odigos-instrumentation=enabled |
There was a problem hiding this comment.
I actually like this for now as another way to test the label migration in v1.0.144. Agree with removing it after that though
| @@ -1,44 +1,34 @@ | |||
| apiVersion: apps/v1 | |||
| kind: Deployment | |||
| metadata: | |||
| annotations: | |||
| deployment.kubernetes.io/revision: "4" | |||
| generation: 5 # Note new generation after annotating service.name, but not new revision | |||
There was a problem hiding this comment.
huge +1 to not having to keep track of the annotation/generation skew anymore!
This PR deprecates the
odigos.io/reported-nameannotation, as part of the effort the reduce the changes we do to workload objects.reportedNamefield to the Source spec. This field is only valid for workloads Sources and not for Namesoace Sources. This is enforced by the Source validation webhook.instrumentor/instrumentationconfigwhich were responsible for updating the instrumentationConfig if the annotation changes.startlangdetectioncontroller which already has a reconciler for Sources but is currently left out of this PR to keep this one dedicated to deprecating the annotation.sourcesandworkload-lifecyclee2e tests. These tests made use of the annotation to differ between the instrumented service during different phases of the tests. Both are updated to use the Sources to achieve that.