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

[connector/servicegraph] Fix failed label does not work leads to servicegraph metrics error #32019

Merged
merged 1 commit into from
Jul 8, 2024
Merged

[connector/servicegraph] Fix failed label does not work leads to servicegraph metrics error #32019

merged 1 commit into from
Jul 8, 2024

Conversation

Frapschen
Copy link
Contributor

@Frapschen Frapschen commented Mar 28, 2024 •
edited
Loading

Description:
resolve: #32018

Link to tracking Issue: fixes #32018

Testing:

add a unit test to verify this bug fixed

Documentation: <Describe the documentation added

@Frapschen Frapschen requested a review from a team March 28, 2024 09:42
@Frapschen Frapschen changed the title [connector/servicegraph] Fix failed label not work [connector/servicegraph] Fix failed label does not work leads to servicegraph metrics error Mar 28, 2024
@github-actions github-actions bot requested a review from mapno March 28, 2024 09:42
@Frapschen Frapschen force-pushed the fix_servicegraph_failed_label_not_work branch from b9ab5d5 to 7c82be5 Compare March 28, 2024 09:51
Frapschen added a commit to openinsight-proj/opentelemetry-collector-contrib that referenced this pull request Mar 29, 2024
@Frapschen
fix servicegraph bug(orgin open-telemetry#32019)
c65b860
Frapschen added a commit to openinsight-proj/opentelemetry-collector-contrib that referenced this pull request Apr 2, 2024
@Frapschen
Merge pull request #18 from openinsight-proj/fix_servicegraph_bug
dfb29e3 
fix servicegraph bug(origin open-telemetry#32019)
mapno
mapno suggested changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@mapno mapno left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation (#32018) and for the fix.

I disagree with the proposed solution. I believe the root cause for this is that the metric has a label failed in the first place. Failure should either be indicated as a label with a single metric (ie. traces_service_graph_request_total), or with a different metric and no label. How it's currently set up is a mix of both, which leaves it a in strange spot.
The way I see forward is doing both approaches.

connector/servicegraphconnector/connector.go Outdated Show resolved Hide resolved
mapno
mapno approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@mapno mapno 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!

Frapschen added a commit to openinsight-proj/opentelemetry-collector-contrib that referenced this pull request Apr 17, 2024
@Frapschen
fix servicegraph bug(orgin open-telemetry#32019)
5b57c79
Frapschen added a commit to openinsight-proj/opentelemetry-collector-contrib that referenced this pull request Apr 22, 2024
@Frapschen
fix servicegraph bug(orgin open-telemetry#32019)
70645bb
@JaredTan95 JaredTan95 mentioned this pull request Apr 23, 2024
Merged
t00mas
t00mas approved these changes May 2, 2024
View reviewed changes
Copy link
Contributor

@t00mas t00mas left a comment

Choose a reason for hiding this comment

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

Just left a minor suggestion, if you don't mind

connector/servicegraphconnector/connector.go Outdated Show resolved Hide resolved
connector/servicegraphconnector/connector_test.go Outdated
@@ -27,6 +27,9 @@ import (
"go.opentelemetry.io/otel/sdk/metric/metricdata"
"go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest"
"go.uber.org/zap/zaptest"

"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/golden"
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that you introduced golden to these tests 💯

@jpkrohling jpkrohling force-pushed the fix_servicegraph_failed_label_not_work branch from ae9cc17 to 6b67c8b Compare May 7, 2024 12:59
@github-actions GitHub Actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label May 22, 2024
@jpkrohling jpkrohling force-pushed the fix_servicegraph_failed_label_not_work branch from 360419f to c8eef99 Compare May 22, 2024 08:36
@github-actions github-actions bot removed the Stale label May 23, 2024
@JaredTan95 JaredTan95 added the ready to merge Code review completed; ready to merge by maintainers label Jun 5, 2024
This was referenced Jun 5, 2024
Closed
Closed
@andrzej-stencel andrzej-stencel removed the ready to merge Code review completed; ready to merge by maintainers label Jun 5, 2024
@andrzej-stencel
Copy link
Member

The connector's unit tests are failing? https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/9347048326/job/25723257817?pr=32019

Error: ./connector_test.go:101:11: assignment mismatch: 1 variable but newConnector returns 2 values

@github-actions GitHub Actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 20, 2024
@Frapschen Frapschen removed the Stale label Jul 3, 2024
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Jul 3, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Frapschen / name: Murphy Chen (37f4f88)

@Frapschen Frapschen force-pushed the fix_servicegraph_failed_label_not_work branch from 4dc1700 to abdea68 Compare July 3, 2024 02:29
@Frapschen Frapschen force-pushed the fix_servicegraph_failed_label_not_work branch from c637b2f to 00d8fb1 Compare July 3, 2024 05:19
@Frapschen @t00mas
Fix failed label does not work leads to servicegraph metrics error
37f4f88 
Co-authored-by: Tomas Pica <tomas.pica@gmail.com>
@Frapschen Frapschen force-pushed the fix_servicegraph_failed_label_not_work branch from 0fec7bb to 37f4f88 Compare July 3, 2024 09:42
@jpkrohling jpkrohling merged commit 2bada71 into open-telemetry:main Jul 8, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 8, 2024
@Frapschen Frapschen deleted the fix_servicegraph_failed_label_not_work branch August 3, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpkrohling jpkrohling jpkrohling approved these changes

@t00mas t00mas t00mas approved these changes

@mapno mapno mapno approved these changes

Assignees

@atoulme atoulme

Labels
cmd/otelcontribcol otelcontribcol command cmd/oteltestbedcol connector/servicegraph
Projects
None yet
Milestone
v0.105.0
Development

Successfully merging this pull request may close these issues.

[connector/servicegraph] failed label does not work leads to servicegraph metrics error
7 participants
@Frapschen @andrzej-stencel @jpkrohling @t00mas @mapno @atoulme @JaredTan95