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 configurable remote_write headers to metrics-generator #3175

Merged
merged 11 commits into from
Jan 30, 2024

Conversation

mapno
Copy link
Member

@mapno mapno commented Nov 23, 2023

What this PR does:

Adds per-tenant configurable headers for remote_write requests in the metrics-generator.

It also adds envvar expansion to the overrides configuration.

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

Checklist

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

modules/generator/storage/instance.go Show resolved Hide resolved
RemoteWriteConfigs: generateTenantRemoteWriteConfigs(s.cfg.RemoteWrite, s.tenantID, newHeaders, s.cfg.RemoteWriteAddOrgIDHeader, s.logger),
})
if err != nil {
level.Error(s.logger).Log("msg", "Failed to update remote write headers. Remote write will continue with old headers", "err", err)
Copy link
Member

Choose a reason for hiding this comment

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

do we know this is true? if it the prom remote storage fails to apply new configs do we know it continues with the old?

updateProcessors at the level above increments a counter metric on failure which i like. metrics_generator_active_processors_update_failed_total. i would love to just have all of this reloaded and counted under the same metric, but that name is pretty opinionated.

Copy link
Member Author

Choose a reason for hiding this comment

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

do we know this is true? if it the prom remote storage fails to apply new configs do we know it continues with the old?

Yes. If the update fails, the function returns and the original queue is left untouched https://github.com/prometheus/prometheus/blob/main/storage/remote/write.go#L148-L177.

updateProcessors at the level above increments a counter metric on failure which i like. metrics_generator_active_processors_update_failed_total. i would love to just have all of this reloaded and counted under the same metric, but that name is pretty opinionated.

Good point. Added!

@mapno mapno force-pushed the generator-remote-write-headers branch from bd8946f to 36ff74e Compare November 29, 2023 13:53
@knylander-grafana
Copy link
Contributor

@mapno Will we need doc updates for this?

@mapno mapno force-pushed the generator-remote-write-headers branch from 36ff74e to ff85f17 Compare January 3, 2024 16:12
@mapno mapno requested a review from joe-elliott January 3, 2024 16:17
@knylander-grafana
Copy link
Contributor

knylander-grafana commented Jan 9, 2024

@mapno It looks like we'll need docs for authentication and configuration for this feature. I'll add a note to the issue as well.

@mapno mapno force-pushed the generator-remote-write-headers branch from 94cfe7c to 55050bf Compare January 12, 2024 08:52
@mapno mapno enabled auto-merge (squash) January 30, 2024 13:19
@mapno mapno merged commit 7a65cc0 into grafana:main Jan 30, 2024
14 checks passed
@mapno mapno deleted the generator-remote-write-headers branch January 30, 2024 13:40
@ToonTijtgat2
Copy link

Dear, thanks for this feature, could it be that the remote_write_headers parameter is not added in the configuration page? https://grafana.com/docs/tempo/latest/configuration/#standard-overrides

@ToonTijtgat2
Copy link

Dear

I'm not sure if I'm doing something wrong, but it does not seem to work for me.
I tried with the legacy config:

      metrics_generator_processors:
      - span-metrics
      - service-graphs
      metrics_generator_remote_write_headers:
        tenant-id: prometheus-apps-app-dev```
        
This shows me in the /status/runtime-config the correct settings.
![image](https://github.com/grafana/tempo/assets/109516706/9739ada2-dda1-4d6e-9147-d2bdb14f3dda)

So I started to try with the new config
I placed it in the conf/tempo.yaml
Placed it on runtime-config/overrides.yaml
placed it under conf/overrides.yaml
It always keeps saying defaults is not found on the legacyoverrides.
The config I try to accomplice looks like this.
```overrides.yaml: |
    defaults:
      global:
        max_bytes_per_trace: 1500000
      metrics_generator:
        processors: [service-graphs, span-metrics]
        remote_write_headers:
          tenant-id: prometheus-apps-app-dev```
          
          
Can someone please help me to set this up correctly, or check what went wrong?

Thanks Toon Tijtgat

@ToonTijtgat2
Copy link

This also does not work:
conf/tempo.yaml
overrides:
per_tenant_override_config: /conf/overrides.yaml
max_bytes_per_trace: 1500000
metrics_generator_processors:
- span-metrics
- service-graphs
conf/overrides.yaml
overrides:
"tracing-apps-app-dev":
metrics_generator:
remote_write_headers:
tenant-id: prometheus-apps-app-dev
But now it gives per-tenant overrides config type does not match static overrides config type

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.

metrics_generator: different header option for remote write
4 participants