Skip to content
This repository was archived by the owner on Jul 28, 2026. It is now read-only.

Parse const_labels correctly in exporter - #1440

Merged
rfratto merged 7 commits into
grafana-cold-storage:mainfrom
fredr:fredr/const-labels
Mar 2, 2022
Merged

Parse const_labels correctly in exporter#1440
rfratto merged 7 commits into
grafana-cold-storage:mainfrom
fredr:fredr/const-labels

Conversation

@fredr

@fredr fredr commented Mar 1, 2022

Copy link
Copy Markdown
Contributor

PR Description

Config field const_labels have different types in the remotewriterexporter than it has in the main config type. This causes an error when starting the agent:

error reading exporters configuration for \"remote_write\": 1 error(s) decoding:\n\n* 'const_labels[0]' has invalid keys: key, some"

With this example config:

server:
  log_level: info
  http_listen_port: 12345

traces:
  configs:
  - name: test
    receivers:
      zipkin:
        endpoint: 0.0.0.0:9999
    remote_write:
      - endpoint: traces.local:5555
        insecure: false
        tls_config:
            insecure_skip_verify: true
    spanmetrics:
      metrics_instance: test
      const_labels:
        some: "somevalue"
        key: "value"

Notes to the Reviewer

The previous implementation also had a bug in the mapstructure field, where both value and name where named name.

PR Checklist

  • CHANGELOG updated
  • Documentation added
  • Tests updated

@fredr
fredr requested review from joe-elliott and mapno as code owners March 1, 2022 09:45
@CLAassistant

CLAassistant commented Mar 1, 2022

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@fredr

fredr commented Mar 1, 2022

Copy link
Copy Markdown
Contributor Author

Test failed with

failed to start manager: inotify_add_watch /sys/fs/cgroup/devices: no space left on device

I think that is unrelated, I can run all the tests locally

@mapno mapno left a comment

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.

Hi! Thanks for addressing this issue, it's an awful bug.

It'd be great if we add const_labels to the test. Please, add an entry to the CHANGELOG :)

Comment thread pkg/traces/remotewriteexporter/exporter.go Outdated
Comment thread pkg/traces/remotewriteexporter/factory.go Outdated
@fredr
fredr force-pushed the fredr/const-labels branch from 9574b7d to fec3832 Compare March 1, 2022 12:52
@fredr

fredr commented Mar 1, 2022

Copy link
Copy Markdown
Contributor Author

It'd be great if we add const_labels to the test.

I added a new test that sets the spanmetrics section and creates a traces collector. Would you rather that I add it to the TestTraces-test?

Please, add an entry to the CHANGELOG :)

Done!

@mapno mapno left a comment

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.

I added a new test that sets the spanmetrics section and creates a traces collector.

Nice! Looks good to me. Thanks!

Would you rather that I add it to the TestTraces-test?

That's fine by me. I think ideally the test should be in config_test.go, but it's fine where it is as long as it's tested. Up to you!

Thanks for the PR and addressing the comments! :)

@rfratto
rfratto merged commit 872e5c9 into grafana-cold-storage:main Mar 2, 2022
@fredr
fredr deleted the fredr/const-labels branch March 2, 2022 13:30
@github-actions github-actions Bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Apr 3, 2024
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants