-
Notifications
You must be signed in to change notification settings - Fork 319
Add test for TracingFilter #2847
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
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM. Thanks @adutra !
.containsEntry("rojo", "00f067aa0ba902b7") | ||
.containsEntry("congo", "t61rcWkgMzE"); |
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.
Nit: using variable or constant here? t61rcWkgMzE
-> congoValue?
.containsEntry("rojo", "00f067aa0ba902b7") | |
.containsEntry("congo", "t61rcWkgMzE"); | |
.containsEntry("rojo", spanId) | |
.containsEntry("congo", "t61rcWkgMzE"); |
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.
The test code LGTM. My comment is more for discussion... I wonder if it might be preferable to move that to the dev
ML 🤔
Map<AttributeKey<?>, Object> attributes = span.getAttributes().asMap(); | ||
assertThat(attributes) | ||
.containsEntry(stringKey(TracingFilter.REALM_ID_ATTRIBUTE), "POLARIS") | ||
.containsEntry(stringKey(TracingFilter.REQUEST_ID_ATTRIBUTE), "12345"); |
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.
side note: my thinking was to reuse request ID from OTel (e.g. trace ID + span ID). Since we're moving towards "correlation", I think it makes sense to integrate with the existing OTel standard. In that case it should not be necessary to add REQUEST_ID_ATTRIBUTE
to the span, but someone inspecting the response headers would be able to correlate them to the trace.
If OTel is present, what is the value of Polaris accepting / generating yet another request ID?
Are we considering the Polaris request ID as a first class feature of the protocol (REST API flowing into Events) as opposed to a "helper" or "observability" feature?
If OTel is not present, we can certainly generate a fresh request ID.
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.
OK, let's move this conversation to the dev ML:
https://lists.apache.org/thread/bb1qyxjt827t3tomv2xp0s1kovxjsp94
TracingFilter
was lacking a proper test.After discussing with @dimas-b on #2757 I decided to create a test to illustrate two things: