Skip to content
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 remoteUser (from http basic auth) and remoteAddr to Thanos slow query logs #6153

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

kfdm
Copy link
Contributor

@kfdm kfdm commented Feb 22, 2023

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

When tracking where a suspicious query came from, one often needs to know the remote address or remote user that made the request. Adding these should help with debugging.

Verification

Built locally and ran locally then queried with http client.

http 'http://localhost:10902/api/v1/query?query=count(up)' --auth foo:bar
level=info ts=2023-02-22T00:29:36.712614Z caller=handler.go:195 org_id=anonymous msg="slow query detected" method=GET host=localhost:10902 path=/api/v1/query remote_user=foo remote_addr=[::1]:54112 time_taken=2.956678125s grafana_dashboard_uid=- grafana_panel_id=- trace_id=- param_query=count(up)

Notes

So this is posted slightly early as a proof of concept to see if the basic idea makes sense to the Thanos team.

It seems like reportQueryStats is not actually called from anywhere, only reportSlowQuery but I've updated both in the interest of keeping things in sync.

@kfdm kfdm force-pushed the frontend-remote-address branch 2 times, most recently from f18afdc to 3b1601f Compare February 22, 2023 00:36
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, one question from my side.

@@ -177,11 +177,15 @@ func (f *Handler) reportSlowQuery(r *http.Request, responseHeaders http.Header,
thanosTraceID = traceID
}

remoteUser, _, _ := r.BasicAuth()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only way we can get the remote user? Are there maybe some Grafana headers we can use as fallback when basic auth is not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grafana can be but one client. It's highly possible that users may use custom clients to query some data from Thanos.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we indicate in the changelog that this only works for basic auth?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added mention of basic auth

Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this makes sense to me as well + @fpetkovski's feedback

…uery logs

When tracking where a suspicious query came from, one often needs to
know the remote address or remote user that made the request. Adding these
should help with debugging.

Signed-off-by: Paul Traylor <paul.traylor@linecorp.com>
@kfdm kfdm changed the title Add remoteUser and remoteAddr to Thanos slow query logs Add remoteUser (from http basic auth) and remoteAddr to Thanos slow query logs Feb 24, 2023
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@saswatamcode saswatamcode merged commit e5563f2 into thanos-io:main Feb 24, 2023
ngraham20 pushed a commit to ngraham20/thanos that referenced this pull request Apr 17, 2023
…uery logs (thanos-io#6153)

When tracking where a suspicious query came from, one often needs to
know the remote address or remote user that made the request. Adding these
should help with debugging.

Signed-off-by: Paul Traylor <paul.traylor@linecorp.com>
ngraham20 pushed a commit to ngraham20/thanos that referenced this pull request Apr 17, 2023
…uery logs (thanos-io#6153)

When tracking where a suspicious query came from, one often needs to
know the remote address or remote user that made the request. Adding these
should help with debugging.

Signed-off-by: Paul Traylor <paul.traylor@linecorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants