-
Notifications
You must be signed in to change notification settings - Fork 47
feat: propagate trace via annnotation for limitador cr & auth configs #1717
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
Conversation
de01221 to
8658079
Compare
|
👍 |
| limitador.Annotations = make(map[string]string) | ||
| } | ||
|
|
||
| otel.GetTextMapPropagator().Inject(ctx, propagation.MapCarrier(limitador.Annotations)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only setting annotation here. Did not also set the annotation at limitador reconciler at:
kuadrant-operator/internal/controller/limitador_reconciler.go
Lines 62 to 84 in a4526a7
| limitador := &limitadorv1alpha1.Limitador{ | |
| TypeMeta: metav1.TypeMeta{ | |
| Kind: "Limitador", | |
| APIVersion: "limitador.kuadrant.io/v1alpha1", | |
| }, | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: kuadrant.LimitadorName, | |
| Namespace: kobj.Namespace, | |
| OwnerReferences: []metav1.OwnerReference{ | |
| { | |
| APIVersion: kobj.GroupVersionKind().GroupVersion().String(), | |
| Kind: kobj.GroupVersionKind().Kind, | |
| Name: kobj.Name, | |
| UID: kobj.UID, | |
| BlockOwnerDeletion: ptr.To(true), | |
| Controller: ptr.To(true), | |
| }, | |
| }, | |
| }, | |
| Spec: limitadorv1alpha1.LimitadorSpec{ | |
| MetricLabelsDefault: ptr.To("descriptors[1]"), | |
| }, | |
| } |
since if we apply the annotation at both these places, one will override the other and cause a reconcile loop. Maybe this is a non-issue since I'm guessing we only care about tracing the reconcile of the limitador limits portion.
But I wonder if we should/want to merge the reconcilers for limitador creation and the limitador limits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good point, I do tend to agree we probably only care about limit reconciles but it does open up the question of why we have both
Signed-off-by: KevFan <chfan@redhat.com>
Signed-off-by: KevFan <chfan@redhat.com>
Signed-off-by: KevFan <kevin_fan@hotmail.co.uk>
|
Updated PR to also inject the trace annotation into auth configs to link traces (Kuadrant/authorino#574 is the related authorino PR) |
adam-cattermole
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Managed to verify this with the other changes and it's working as it's intended.
I don't think it's necessarily tied to this PR; but we might need to revisit this solution - once we set the annotation all reconciles whether they were triggered by the kuadrant-operator, other changes in the cluster, or users changing the CR will be linked to the last time we set the traceparent. I don't think this is necessarily a bad thing as it's still providing the link to the last time the kuadrant-operator updated it; but the source of that specific reconcile or change may not be at all related.
Yeah, that's the only thing with this. The |
Description
Closes: #1690
Requires: Kuadrant/limitador-operator#222 Kuadrant/authorino#574
Limitador Operator -> Kuadrant Operator


Authorino -> Kuadrant Operator

