Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings
This repository was archived by the owner on Jul 19, 2023. It is now read-only.

Allow the service_name label to exist #739

Merged
merged 2 commits into from
Jun 6, 2023
Merged

Allow the service_name label to exist #739

merged 2 commits into from
Jun 6, 2023

Conversation

0xPoe
Copy link
Contributor

@0xPoe 0xPoe commented Jun 1, 2023 •
edited
Loading

close #733

If the service_name is present, utilize it and maintain appname as a label.

  • Manual test

@0xPoe 0xPoe changed the title Allow the service_name label to exist WIP: Allow the service_name label to exist Jun 1, 2023
0xPoe added 2 commits June 6, 2023 08:47
@0xPoe
Fix typo
4d49907 
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
@0xPoe
Allow the service_name label to exist
9075dfa 
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
@0xPoe 0xPoe force-pushed the rustin-patch-service-name branch from bd2d2df to 9075dfa Compare June 6, 2023 00:47
@0xPoe
Copy link
Contributor Author

0xPoe commented Jun 6, 2023

Test locally:
Before:

    let agent = PyroscopeAgent::builder("http://localhost:4100", "example.basic")
        .backend(pprof_backend(PprofConfig::new().sample_rate(100)))
        .tags(
            [
                ("TagA", "ValueA"),
                ("TagB", "ValueB"),
                ("service_name", "test"),
            ]
            .to_vec(),
        )
        .build()?;
level=error caller=ingest_handler.go:57 ts=2023-06-06T01:43:10.130417Z msg="pyroscope ingest" err="pyroscopeIngesterAdapter failed to push: invalid_argument: profile with labels '{TagA=\"ValueA\", TagB=\"ValueB\", __delta__=\"false\", __name__=\"process_cpu\", pyroscope_spy=\"pyroscope-rs\", service_name=\"example.basic\", service_name=\"test\"}' has duplicate label name: 'service_name'"

After:
image
image

@0xPoe 0xPoe marked this pull request as ready for review June 6, 2023 01:47
@0xPoe 0xPoe changed the title WIP: Allow the service_name label to exist Allow the service_name label to exist Jun 6, 2023
@0xPoe
Copy link
Contributor Author

0xPoe commented Jun 6, 2023

@cyriltovena Could you please take a look? Thanks!

@petethepig petethepig self-requested a review June 6, 2023 05:03
petethepig
petethepig approved these changes Jun 6, 2023
View reviewed changes
Copy link
Contributor

@petethepig petethepig left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Thanks for fixing this case!

@cyriltovena cyriltovena merged commit 62b2679 into grafana:main Jun 6, 2023
@0xPoe 0xPoe deleted the rustin-patch-service-name branch June 6, 2023 09:10
simonswine pushed a commit to simonswine/pyroscope that referenced this pull request Jun 30, 2023
@0xPoe
Allow the service_name label to exist (grafana/phlare#739)
6ebe014 
* Fix typo

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

* Allow the `service_name` label to exist

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

---------

Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@petethepig petethepig petethepig approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

When ingesting with duplicate labels, send error back to the client

3 participants

@0xPoe @petethepig @cyriltovena
Morty Proxy This is a proxified and sanitized view of the page, visit original site.