Skip to content

[processor/servicegraph] Fixes the number of bucket counts#16025

Merged
bogdandrutu merged 7 commits into
open-telemetry:mainfrom
JaredTan95:fix_service_graph_out_range
Nov 29, 2022
Merged

[processor/servicegraph] Fixes the number of bucket counts#16025
bogdandrutu merged 7 commits into
open-telemetry:mainfrom
JaredTan95:fix_service_graph_out_range

Conversation

@JaredTan95

Copy link
Copy Markdown
Member

Description:

fix #16000

Link to tracking Issue:

Testing:

Documentation:

@JaredTan95 JaredTan95 requested a review from a team November 2, 2022 04:12
@JaredTan95 JaredTan95 requested a review from a team November 2, 2022 04:12
Signed-off-by: Jared Tan <jian.tan@daocloud.io>
Signed-off-by: Jared Tan <jian.tan@daocloud.io>
@JaredTan95 JaredTan95 force-pushed the fix_service_graph_out_range branch from 9229b37 to 9057817 Compare November 2, 2022 04:20
@JaredTan95 JaredTan95 changed the title [servicegraphprocessor] Fixes the number of bucket counts [processor/servicegraph] Fixes the number of bucket counts Nov 2, 2022
@JaredTan95

Copy link
Copy Markdown
Member Author

@mapno hi, do you have time to help review this?

Signed-off-by: Jared Tan <jian.tan@daocloud.io>

@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.

Nice catch

@JaredTan95

Copy link
Copy Markdown
Member Author

@jpkrohling hi, do you have time to help review this PR?

@MovieStoreGuy

Copy link
Copy Markdown
Contributor

One thing I wanted to double check, looking at the associated issue, I can see that the the problem happens at line 331 of processor.go (I forgot to grab a link to the line).

Are these buckets always going to have a fixed size of N+1 values?

It does concern me that there is no check to see if the index is in the bounds of the slice, since this theoretically happen again.

@JaredTan95

JaredTan95 commented Nov 10, 2022

Copy link
Copy Markdown
Member Author

One thing I wanted to double check, looking at the associated issue, I can see that the the problem happens at line 331 of processor.go (I forgot to grab a link to the line).

Are these buckets always going to have a fixed size of N+1 values?

It does concern me that there is no check to see if the index is in the bounds of the slice, since this theoretically happen again.

Fully tested, there are no out-of-scope errors indeed, it might be a check policy in

dpDuration.BucketCounts().FromRaw(p.reqDurationSecondsBucketCounts[key])
, I guess

JaredTan95 added a commit to openinsight-proj/opentelemetry-collector-contrib that referenced this pull request Nov 14, 2022
Signed-off-by: Jared Tan <jian.tan@daocloud.io>
JaredTan95 added a commit to openinsight-proj/opentelemetry-collector-contrib that referenced this pull request Nov 14, 2022
Signed-off-by: Jared Tan <jian.tan@daocloud.io>

Signed-off-by: Jared Tan <jian.tan@daocloud.io>
@github-actions

Copy link
Copy Markdown
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 Nov 24, 2022
@panpan0000

Copy link
Copy Markdown

Any experter can follow up with this change?

Signed-off-by: Jared Tan <jian.tan@daocloud.io>
@github-actions github-actions Bot added the processor/servicegraph Service graph processor label Nov 24, 2022
@github-actions github-actions Bot requested a review from mapno November 24, 2022 09:32
@JaredTan95

Copy link
Copy Markdown
Member Author

@MovieStoreGuy hi, I added some UT to test it, do you have time to help review this? We are eager to use the latest patches as soon as possible :-P

Signed-off-by: Jared Tan <jian.tan@daocloud.io>
@jpkrohling

Copy link
Copy Markdown
Member

Looks like the CI failed:

Error: processor_test.go:315:29: tc.reqDurationBounds undefined (type struct{caseStr string; duration float64} has no field or method reqDurationBounds) (typecheck)
			p.reqDurationBounds = tc.reqDurationBounds

Signed-off-by: Jared Tan <jian.tan@daocloud.io>
@JaredTan95

Copy link
Copy Markdown
Member Author

Looks like the CI failed:

Error: processor_test.go:315:29: tc.reqDurationBounds undefined (type struct{caseStr string; duration float64} has no field or method reqDurationBounds) (typecheck)
			p.reqDurationBounds = tc.reqDurationBounds

resolved.

@github-actions github-actions Bot removed the Stale label Nov 25, 2022
@JaredTan95

Copy link
Copy Markdown
Member Author

@jpkrohling hi, pls help review this. :-P

@bogdandrutu bogdandrutu merged commit 81b38ec into open-telemetry:main Nov 29, 2022
@JaredTan95 JaredTan95 deleted the fix_service_graph_out_range branch November 29, 2022 06:09
shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2022
…metry#16025)

* Fixes the number of explicit bucket counts.

Signed-off-by: Jared Tan <jian.tan@daocloud.io>

* add changelog

Signed-off-by: Jared Tan <jian.tan@daocloud.io>

* fix UT.

Signed-off-by: Jared Tan <jian.tan@daocloud.io>

* add UT

Signed-off-by: Jared Tan <jian.tan@daocloud.io>

* polish

Signed-off-by: Jared Tan <jian.tan@daocloud.io>

* fix ut

Signed-off-by: Jared Tan <jian.tan@daocloud.io>

Signed-off-by: Jared Tan <jian.tan@daocloud.io>
@plantfansam plantfansam mentioned this pull request Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

processor/servicegraph Service graph processor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[servicegraphprocessor] Index out of range panic in updateDurationMetrics method

6 participants