-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[fix][query] Filter out tracing for access to static UI assets #6374
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6374 +/- ##
==========================================
- Coverage 96.20% 96.18% -0.03%
==========================================
Files 361 361
Lines 20600 20604 +4
==========================================
- Hits 19819 19818 -1
- Misses 597 601 +4
- Partials 184 185 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cmd/query/app/server.go
Outdated
@@ -219,6 +222,12 @@ func createHTTPServer( | |||
MeterProvider: telset.MeterProvider, | |||
}, | |||
handler, | |||
xconfighttp.WithOtelHTTPOptions( | |||
otelhttp.WithFilter(func(r *http.Request) bool { | |||
ignorePaths := regexp.MustCompile(`^/static/.*`) |
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.
^/static/.*
does not require regex, can be matched withstrings.HasPrefix
- it doesn't look like it will work if the UI is running with a custom base path, we need to account for that too
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.
@yurishkuro for 2 - do we want to do a contains
to see if the /static
path is anywhere in the URL?
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.
that would be a bit fuzzy. I would rather use HasPrefix with basePath + "/static/"
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.
done
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.
git diff cmd/jaeger/config.yaml
jaeger_query:
+ base_path: /foobar
storage:
traces: some_store
traces_archive: another_store
$ go run ./cmd/jaeger --config ./cmd/jaeger/config.yaml
Reload UI a few times, observe span /foobar/static/favicon-BxcVf0am.ico
So, not solved.
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.
@yurishkuro apologies - i forgot to push my commits
Signed-off-by: Mahad Zaryab <[email protected]>
Which problem is this PR solving?
Description of the changes
xconfighttp
to ignore accesses to static UI assets at/static/*
Testing
I ran the monitor example and accessed the Jaeger UI to test that the traces were getting recorded on main but not in this PR.
On main
This PR
Checklist
jaeger
:make lint test
jaeger-ui
:npm run lint
andnpm run test