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 possibility to add fanout metadata #490

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GiedriusS
Copy link
Member

Add a mechanism for adding fanout metadata. This will allow surfacing data that so far is only available in traces and/or logs.

GiedriusS added a commit to thanos-io/thanos that referenced this pull request Oct 3, 2024
Add fanout metadata to Analyze() output. Depends on
thanos-io/promql-engine#490.

Signed-off-by: Giedrius Statkevičius <[email protected]>
@GiedriusS GiedriusS marked this pull request as ready for review October 4, 2024 13:38
Add a mechanism for adding fanout metadata. This will allow surfacing
data that so far is only available in traces and/or logs.

Signed-off-by: Giedrius Statkevičius <[email protected]>
@GiedriusS GiedriusS force-pushed the add_fanout_metadata branch from 72a3bb1 to 148f357 Compare October 4, 2024 13:41
@@ -14,6 +15,44 @@ import (
"github.com/thanos-io/promql-engine/query"
)

type metadataKey string
Copy link
Contributor

Choose a reason for hiding this comment

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

we have similar pattern in /execution/warning i think, should we move this to /execution/metadata so its similarly structured?

@@ -37,7 +37,7 @@ func newRelabelOperator(
next: next,
funcExpr: funcExpr,
}
oper.OperatorTelemetry = model.NewTelemetry(oper, opts)
oper.OperatorTelemetry = model.NewTelemetry(context.Background(), oper, opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use context from constructor and pass through from engine? This is a bit more work but feels cleaner, we do same with warnings fwiw - if we want to use different kind of metadata someday

Copy link
Contributor

@MichaHoffmann MichaHoffmann left a comment

Choose a reason for hiding this comment

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

love the idea! lgtm, just 2 comments

return v.(*FanoutMetadata)
}

type FanoutMetadata struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could just be "Metadata" as its just a map; applications outside could build on top of this map since "fanout" is somewhat a thanos concept that this engine doesnt know about; its more an implementation detail of Select. Thanos could use this to just add fan out metadata in its Select implementation but no need that this library knows what this key contains really.

Copy link
Contributor

@MichaHoffmann MichaHoffmann left a comment

Choose a reason for hiding this comment

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

On second thought, couldnt this be implemented completely in thanos? As in "create context, add stuff during select, read out in api and add to analysis" kinda way? If we made sure to use same context for "Select" that was used to create the query?
Kinda like we already pass warnings for partial response.

@GiedriusS
Copy link
Member Author

On second thought, couldnt this be implemented completely in thanos? As in "create context, add stuff during select, read out in api and add to analysis" kinda way? If we made sure to use same context for "Select" that was used to create the query? Kinda like we already pass warnings for partial response.

How will you attach select metadata to individual operators?

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.

3 participants