-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add limitador tracing-endpoint command #130
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #130 +/- ##
==========================================
- Coverage 86.03% 84.94% -1.09%
==========================================
Files 19 19
Lines 988 990 +2
==========================================
- Hits 850 841 -9
- Misses 91 97 +6
- Partials 47 52 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Just one comment/question, but LGTM as it is now
api/v1alpha1/limitador_types.go
Outdated
@@ -324,6 +327,10 @@ type Ports struct { | |||
GRPC int32 `json:"grpc,omitempty"` | |||
} | |||
|
|||
type Tracing struct { | |||
Endpoint *string `json:"endpoint,omitempty"` |
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.
Should endpoint
be optional or required?
spec.tracing
is optional, but spec.tracing.endpoint
?
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 actually, if the tracing struct is provided the endpoint should be required I guess
Should we add come documentation regarding tracing?? Any requirement on the tracing backend side?? Opentelemetry compatible service? |
Yep I think I should definitely add some docs about configuring here, I'll add a follow up commit with that change |
cbdf3d8
to
0e48f7b
Compare
0e48f7b
to
e7ae162
Compare
@eguzki - I'm going to merge this if you're happy with the latest changes |
Changes
A new
--tracing-endpoint
command was added to limitador in Kuadrant/limitador#261. I have used a struct to match the spec in Authorino (eventually to contain anInsecure
boolean once limitador supports TLS tracing endpoint).docker run --rm -t quay.io/kuadrant/limitador:latest limitador-server --help
Verification Steps
Deploy cluster:
Create limitador CR with
.spec.tracing.endpoint
set:Wait for deployment:
kubectl wait --timeout=300s --for=condition=Ready limitador limitador
Check limitador deployment command: