Skip to content

Format Referer header on X-Dashboard-Title and X-Panel-Title#1433

Open
velom wants to merge 2 commits intografana:mainfrom
velom:referer
Open

Format Referer header on X-Dashboard-Title and X-Panel-Title#1433
velom wants to merge 2 commits intografana:mainfrom
velom:referer

Conversation

@velom
Copy link
Copy Markdown

@velom velom commented Nov 6, 2025

The plugin can forward HTTP headers.
Unfortunately, ClickHouse stores only Referer header in the system.query_log.
This PR adds additional header Referer based on X-Dashboard-Title and X-Panel-Title.

Examle of how it looks in the query_log:

    ┌─http_referer────────────────────────────────────────────────┐
 1. │ Explorer                                                    │
 2. │ Dashboard title: "New dashboard", pannel title: "New panel" │
    └─────────────────────────────────────────────────────────────┘

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Nov 6, 2025

CLA assistant check
All committers have signed the CLA.

@adamyeats adamyeats moved this from Incoming to In Progress in Partner Datasources Nov 7, 2025
@adamyeats adamyeats moved this from In Progress to Needs Review in Partner Datasources Nov 7, 2025
@velom
Copy link
Copy Markdown
Author

velom commented Nov 12, 2025

Hi @adamyeats, any ETA here?

Copy link
Copy Markdown
Collaborator

@SpencerTorres SpencerTorres left a comment

Choose a reason for hiding this comment

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

I like this, but is it possible to use the array-like behavior of the headers to add more data? I want to make it easier to fetch this out of the query log. The string formatting works, but I'm wondering if we can make it more HTTP-compliant

What I mean is, is it possible where the header can have multiple entries? I forgot the interface in Go, but there should be a way to do that

@velom
Copy link
Copy Markdown
Author

velom commented Nov 25, 2025

There is one global problem: Grafana doesn't pass most of headers to the datasource plugin, only few number of them, and there are no common HTTP headers.
As far as I understand, there could be this set of headers: https://github.com/grafana/grafana/blob/5694d6371ee9989409f7259295b1cf821cd1f022/pkg/services/query/query.go#L34-L42

I can adjust the header handler and make it configurable:

  1. Header format string
  2. Array of headers to pass to the string

E.g. instead of "Dashboard title: %q, panel title: %q" some users may want to have:

  1. "%q (%q) from %q (%q)"
  2. [X-Panel-Title, X-Panel-Id, X-Dashboard-Title, X-Dashboard-Uid]

What do you think?

@adamyeats adamyeats moved this from Needs Review to Waiting in Partner Datasources Nov 27, 2025
Copy link
Copy Markdown
Collaborator

@SpencerTorres SpencerTorres left a comment

Choose a reason for hiding this comment

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

I suppose this is better than what we have now, I just wish we had a better way to attach metadata to queries. This doesn't apply to TCP connections does it?

if dashboard != "" || panel != "" {
httpHeaders[headerReferer] = fmt.Sprintf("Dashboard title: %q, panel title: %q", dashboard, panel)
} else {
httpHeaders[headerReferer] = "Explorer"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is technically untrue; the query could also come from alerting. You should be able to validate this by checking the uname from the plugin context for grafana_scheduler.

Copy link
Copy Markdown
Contributor

@aangelisc aangelisc left a comment

Choose a reason for hiding this comment

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

Generally this LGTM. I wonder if we should have this configurable - I'm not sure all users would want this enabled, wdyt?

@velom
Copy link
Copy Markdown
Author

velom commented Dec 10, 2025

Sure, sounds reasonable.
Let me make it more flexible like described above, there will be a way to disable it at all.

@aangelisc
Copy link
Copy Markdown
Contributor

Hi @velom 😊 sorry for the delay here. Are you able to update this PR based on my feedback? I believe this should be configurable to allow users to disable it if they wish. I think this should be disabled by default. Also, we should correctly identify alerting queries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Waiting

Development

Successfully merging this pull request may close these issues.

5 participants