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.

Sign up for GitHub

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

Jump to bottom

add host info to userconfigurable overrides api #4854

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

add host info to userconfigurable overrides api #4854

wants to merge 6 commits into from

Conversation

rlankfo
Copy link
Member

@rlankfo rlankfo commented Mar 14, 2025 •
edited
Loading

What this PR does:
This PR adds support for the host info processor to the user configurable overrides api.

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@rlankfo rlankfo changed the title wip: add host info to userconfigurable overrides api WIP: add host info to userconfigurable overrides api Mar 14, 2025
@rlankfo rlankfo self-assigned this Mar 14, 2025
jcreixell
jcreixell reviewed Mar 14, 2025
View reviewed changes
modules/overrides/userconfigurable/api/api_test.go
@@ -62,7 +62,7 @@ func Test_UserConfigOverridesAPI_overridesHandlers(t *testing.T) {
name: "GET",
handler: overridesAPI.GetHandler,
req: prepareRequest(tenant, "GET", nil),
expResp: `{"forwarders":["my-other-forwarder"],"cost_attribution":{},"metrics_generator":{"processor":{"service_graphs":{},"span_metrics":{}}}}`,
Copy link

@jcreixell jcreixell Mar 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

we can add a test here, set some pre-defined values in overridesAPI and extend the postJSON payload to include different values. I was particularly interested on whether setting an empty list of host identifiers and an empty metric name would override previous values. Seems like it does but having test coverage would be nice. (I was interested in that particular case because it would allow us to disable host info metric generation on a tenant to tenant basis without needing an extra boolean).

jcreixell
jcreixell reviewed Mar 14, 2025
View reviewed changes
Copy link

@jcreixell jcreixell left a comment

Choose a reason for hiding this comment

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

added a small comment but overall LGTM

@rlankfo rlankfo marked this pull request as ready for review March 17, 2025 17:47
@rlankfo rlankfo changed the title WIP: add host info to userconfigurable overrides api add host info to userconfigurable overrides api Mar 17, 2025
joe-elliott
joe-elliott reviewed Mar 18, 2025
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment •
edited
Loading

Choose a reason for hiding this comment

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

do we need to add code here so that the host info processor reloads correctly?

https://github.com/grafana/tempo/blob/main/modules/generator/instance.go#L277

here as well?

https://github.com/grafana/tempo/blob/main/modules/generator/config.go#L157

@rlankfo rlankfo force-pushed the rlankfo/host-info-processor-overrides branch from 5ef2397 to ca57d2d Compare March 20, 2025 04:39
t00mas
t00mas reviewed Mar 20, 2025
View reviewed changes
modules/generator/instance_test.go Outdated
hostinfo.Name: {},
}
overrides.hostInfoHostIdentifiers = []string{"host.id"}
overrides.hostInfoMetricName = "traces_host_info"
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit because I've tested and still works) but this test uses the defaults and it should be using non-defaults just in case

joe-elliott
joe-elliott reviewed Mar 21, 2025
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

one concern, but other than that i think this is ready to merge

modules/generator/config.go Outdated
@@ -223,6 +223,10 @@ func (cfg *ProcessorConfig) copyWithOverrides(o metricsGeneratorOverrides, userI

copyCfg.ServiceGraphs.EnableVirtualNodeLabel = o.MetricsGeneratorProcessorServiceGraphsEnableVirtualNodeLabel(userID)

copyCfg.HostInfo.HostIdentifiers = o.MetricsGeneratorProcessorHostInfoHostIdentifiers(userID)
Copy link
Member

Choose a reason for hiding this comment

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

i know you've just copied this from the above lines, but this is a bug b/c it always overrides the config provided to tempo with the user overrides. there is an issue about this somewhere but i can't find it.

can we conditionally overwrite the config only if there is an actual provided value from the overrides? preferably by returning a nil pointer to mean: "there is no value".

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Joe. I added the checks here similar to other overrides I saw in this method. Let me know if this is what you had in mind

@rlankfo rlankfo force-pushed the rlankfo/host-info-processor-overrides branch from ca57d2d to 06ecf4e Compare March 21, 2025 18:51
@knylander-grafana
Copy link
Contributor

Should we add host info to the doc for the user-configurable overrides?

joe-elliott
joe-elliott approved these changes Mar 24, 2025
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott 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 Robbie.

Will wait on @knylander-grafana 's doc request before merging.

@rlankfo
Copy link
Member Author

rlankfo commented Mar 24, 2025 •
edited
Loading

Should we add host info to the doc for the user-configurable overrides?

updated the docs -- my editor automatically removed the $ shell indicator in the shell blocks. I didn't add it back as I noticed some blocks* had it, but others did not. It makes sense to me to remove it as when you click copy you wouldn't want this character included.

knylander-grafana
knylander-grafana approved these changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

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

Thank you for updating the doc!

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

@jcreixell jcreixell jcreixell left review comments

@joe-elliott joe-elliott joe-elliott approved these changes

@t00mas t00mas t00mas approved these changes

@knylander-grafana knylander-grafana knylander-grafana approved these changes

@mdisibio mdisibio Awaiting requested review from mdisibio mdisibio is a code owner

@mapno mapno Awaiting requested review from mapno mapno is a code owner

@yvrhdn yvrhdn Awaiting requested review from yvrhdn yvrhdn is a code owner

@zalegrala zalegrala Awaiting requested review from zalegrala zalegrala is a code owner

@electron0zero electron0zero Awaiting requested review from electron0zero electron0zero is a code owner

@ie-pham ie-pham Awaiting requested review from ie-pham ie-pham is a code owner

@stoewer stoewer Awaiting requested review from stoewer stoewer is a code owner

@javiermolinar javiermolinar Awaiting requested review from javiermolinar javiermolinar is a code owner

@carles-grafana carles-grafana Awaiting requested review from carles-grafana carles-grafana is a code owner

Assignees

@rlankfo rlankfo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rlankfo @knylander-grafana @jcreixell @joe-elliott @t00mas