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 [default] option to default the project_id label for exemplars #449

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

realschwa
Copy link
Collaborator

@realschwa realschwa commented Mar 23, 2023

This PR allows some defaulting logic for the label project_id for exemplars. This is so users only have to supply a span_id and trace_id for traces to be compatible with Cloud Trace since Cloud Trace traces need a spanID, traceID, and projectID that is stored as a SpanContext. See https://cloud.google.com/trace/docs/setup#create-exemplars

If export.exemplars.populate-exemplar-project-id is explicitly set to false, then we will not auto-populate project_id, but the user can still explicitly attach the label to the exemplar if they choose. This will happen regardless of whether or not export.exemplars.project-id is set.

If export.exemplars.project-id is set AND export.exemplars.populate-exemplar-project-id is set to true (the default value) then we will auto-populate project_id with what they supplied.

If export.exemplars.populate-exemplar-project-id is set to true and export.exemplars.project-id is not supplied, then we will use the user's projectId that they are sending metrics to. This will be the default state if nothing is done by the user.

If the user ever explicitly provides project_id in the LabelSet of an exemplar, then we will always use the provided value here, regardless of the new options.

@realschwa realschwa changed the title Add option to default the project_id label for exemplars [NEED TO ADD UNIT TESTS] Add option to default the project_id label for exemplars Mar 23, 2023
@realschwa realschwa marked this pull request as draft March 23, 2023 12:40
@realschwa realschwa changed the title [NEED TO ADD UNIT TESTS] Add option to default the project_id label for exemplars Add option to default the project_id label for exemplars Mar 23, 2023
@realschwa realschwa marked this pull request as ready for review March 24, 2023 17:18
@realschwa realschwa changed the title Add option to default the project_id label for exemplars Add [default] option to default the project_id label for exemplars Mar 24, 2023
@@ -170,6 +170,12 @@ type ExporterOpts struct {
Location string
Cluster string

// If true, automatically populate the project id in an exemplar labelset.
PopulateExemplarProjectID bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say let's avoid bools flag. They are not flexible and scalable. What about empty ExemplarProjectID stopping this to populate that extra label?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the semantics of this would get confusing since this is supposed to be both overridable and disableable.

An empty ExemplarProjectID seems to make sense as the case where you would not autopopulate the project_id, but not passing something in seems to imply that it is the default state which is not what we want. We want the default state to be to autopopulate the exemplar projectID.

Unless you mean empty as in the literal empty string ("") which also seems to be semantically confusing.

I'll defer to you, @pintohutch and @lyanco on this one.

@@ -133,6 +133,12 @@ func FromFlags(a *kingpin.Application, userAgentProduct string) func(log.Logger,
a.Flag("export.label.project-id", fmt.Sprintf("Default project ID set for all exported data. Prefer setting the external label %q in the Prometheus configuration if not using the auto-discovered default.", export.KeyProjectID)).
Default(opts.ProjectID).StringVar(&opts.ProjectID)

a.Flag("export.exemplars.populate-exemplar-project-id", "If true, automatically populate the 'project_id' label in an exemplar labelset.").
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Cool, proposed a small simplification of UX, otherwise good 👍🏽

@realschwa realschwa requested review from bwplotka and pintohutch and removed request for pintohutch and TheSpiritXIII March 28, 2023 14:38
@realschwa realschwa marked this pull request as draft March 29, 2023 14:47
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.

None yet

2 participants