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

chore: add query-frontend option to select request headers in query logs #3383

Merged

Conversation

jmichalek132
Copy link
Contributor

@jmichalek132 jmichalek132 commented Feb 12, 2024

What this PR does:

Adding feature present in mimir and Loki, specifically grafana/mimir#5030 and grafana/loki#11422.

Adds a config option to the query-frontend to specify a list of request headers to include in query logs.

For example, setting -query-frontend.log-query-request-headers="X-Grafana-Org-Id" and sending a query with X-Grafana-Org-Id:1 results in query log lines that include header_x_grafana_org_id=1.

Which issue(s) this PR fixes:
Fixes #3342

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented Feb 12, 2024

CLA assistant check
All committers have signed the CLA.

@jmichalek132 jmichalek132 force-pushed the jm-chore-frontend-log-arbitrary-headers branch from fa6eae7 to 04ec072 Compare February 12, 2024 17:17
@jmichalek132
Copy link
Contributor Author

For the pipeline failure, the test fails looks unrelated to my change:

tenant=0 component=remote level=info remote_name=ce2f78 url=http://127.0.0.1:35569/receive msg="Scraped metadata watcher stopped"
    instance_test.go:140: 
        	Error Trace:	/home/runner/work/tempo/tempo/modules/generator/storage/instance_test.go:140
        	            				/home/runner/work/tempo/tempo/modules/generator/storage/instance_test.go:340
        	            				/opt/hostedtoolcache/go/1.21.6/x64/src/runtime/asm_amd64.s:1650
        	Error:      	Received unexpected error:
        	            	write /tmp/TestInstance_multiTenancy3811487438/001/0/wal/00000000: file already closed
        	Test:       	TestInstance_multiTenancy
tenant=0 component=remote level=info remote_name=ce2f78 url=http://127.0.0.1:35569/receive msg="Remote storage stopped."

For the linting I ran make lint locally as instructed by the contributing guide, but it came up with many complaints for code I didn't touch, so unsure how to address that.

Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

LGTM, only to minor things.

modules/frontend/handler.go Outdated Show resolved Hide resolved
modules/frontend/handler.go Outdated Show resolved Hide resolved
@joe-elliott
Copy link
Member

For the pipeline failure, the test fails looks unrelated to my change:

Don't worry about this. CI flakiness. We can ignore this error.

Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@mapno
Copy link
Member

mapno commented Feb 23, 2024

Ups, it seems the logQueryRequestHeaders wasn't correctly updated everywhere. When that's fixed I'll merge cc/ @jmichalek132

@jmichalek132
Copy link
Contributor Author

Ups, it seems the logQueryRequestHeaders wasn't correctly updated everywhere. When that's fixed I'll merge cc/ @jmichalek132

Sorry for the delay, fixed now.

@mapno mapno merged commit 3bb0c2c into grafana:main Feb 26, 2024
15 checks passed
kvrhdn pushed a commit to kvrhdn/tempo that referenced this pull request Feb 26, 2024
…ogs (grafana#3383)

* chore: add query-frontend option to select request headers in query logs

* chore: added changelog entry

* Update modules/frontend/handler.go

Co-authored-by: Mario <[email protected]>

* Update modules/frontend/handler.go

Co-authored-by: Mario <[email protected]>

* chore: fix typo in a name

---------

Co-authored-by: Mario <[email protected]>
@jmichalek132 jmichalek132 deleted the jm-chore-frontend-log-arbitrary-headers branch February 27, 2024 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add query frontend stats logs to tempo
4 participants