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

Fix broken spanmetrics counters after span producing service restart #29711

Merged
merged 18 commits into from
Dec 21, 2023
Merged

Fix broken spanmetrics counters after span producing service restart #29711

merged 18 commits into from
Dec 21, 2023

Conversation

portertech
Copy link
Contributor

@portertech portertech commented Dec 8, 2023 •
edited
Loading

My spanmetrics counters (e.g. calls_total) break after restarting the span producing service.

For example:

Screenshot from 2023-12-06 11-39-57

I discovered that the resource key used for the calculated metrics was a map hash of the resource attributes. This worked fine for some instrumented services, however, other services include attributes like its process id etc. Restarting one of these services would result in a new hash and calculated resource metrics (in addition to the existing ones).

This pull-request filters the resource attributes used to produce the resource metrics key map hash. I am now able to restart services without breaking my counters.

@portertech
filter resource attributes for resourceKey map hash
1678b4a 
don't include things like process pid etc.

Signed-off-by: Sean Porter <portertech@gmail.com>
@portertech portertech requested review from a team and MovieStoreGuy December 8, 2023 18:23
songy23
songy23 reviewed Dec 8, 2023
View reviewed changes
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

Please add a changelog

@portertech
used a selected resource key hash attribute in tests
47c7b62 
Signed-off-by: Sean Porter <portertech@gmail.com>
@portertech
changelog entry
31d759b 
Signed-off-by: Sean Porter <portertech@gmail.com>
albertteoh
albertteoh reviewed Dec 11, 2023
View reviewed changes
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

Thanks! I think this might also be useful in limiting memory/cpu resources; which seems to be coming up as issues recently.

Added a couple of comments.

connector/spanmetricsconnector/connector.go Outdated Show resolved Hide resolved
connector/spanmetricsconnector/connector.go Outdated Show resolved Hide resolved
@portertech
optimized resource key attr lookup
d796051 
Signed-off-by: Sean Porter <portertech@gmail.com>
@portertech
configurable resource metrics key attributes
4bd8faf 
Signed-off-by: Sean Porter <portertech@gmail.com>
@portertech
linter fix, i += 1 -> i++
7ae229f 
Signed-off-by: Sean Porter <portertech@gmail.com>
@portertech
updated changelog to indicate an enhancement
89d0a41 
Signed-off-by: Sean Porter <portertech@gmail.com>
albertteoh
albertteoh approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@albertteoh albertteoh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good overall! Just some nits.

Thanks for making it configurable by the way!

connector/spanmetricsconnector/config.go Show resolved Hide resolved
connector/spanmetricsconnector/connector.go Outdated Show resolved Hide resolved
connector/spanmetricsconnector/connector.go Outdated Show resolved Hide resolved
connector/spanmetricsconnector/connector_test.go Show resolved Hide resolved
albertteoh
albertteoh approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

Thanks!

MovieStoreGuy
MovieStoreGuy reviewed Dec 14, 2023
View reviewed changes
Copy link
Contributor

@MovieStoreGuy MovieStoreGuy left a comment

Choose a reason for hiding this comment

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

Just some minor optimisations and clarification have behaviour, otherwise, seems reasonable to me.

connector/spanmetricsconnector/config.go Show resolved Hide resolved
connector/spanmetricsconnector/connector.go Outdated Show resolved Hide resolved
@MovieStoreGuy MovieStoreGuy added the ready to merge Code review completed; ready to merge by maintainers label Dec 18, 2023
@github-actions github-actions bot mentioned this pull request Dec 19, 2023
Closed
@portertech portertech mentioned this pull request Dec 19, 2023
Closed
@codeboten codeboten merged commit 4acf9f8 into open-telemetry:main Dec 21, 2023
85 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 21, 2023
@portertech portertech deleted the spanmetrics-broken-count branch December 21, 2023 19:01
@portertech portertech restored the spanmetrics-broken-count branch January 8, 2024 19:01
@portertech portertech mentioned this pull request Jan 10, 2024
Merged
djaglowski pushed a commit that referenced this pull request Jan 10, 2024
@portertech
[connector/spanmetrics] Added @portertech to codeowners (#30401)
b2fdfec 
Added @portertech (me) to connector/spanmetrics codeowners.

Supporters: @djaglowski, @albertteoh 

Recent relevant PR:
#29711

Signed-off-by: Sean Porter <portertech@gmail.com>
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Jan 10, 2024
@portertech @albertteoh @cparkins
Fix broken spanmetrics counters after span producing service restart (o…
7359094 
…pen-telemetry#29711)

My spanmetrics counters (e.g. `calls_total`) break after restarting the
span producing service.

For example:

![Screenshot from 2023-12-06
11-39-57](https://github.com/open-telemetry/opentelemetry-collector-contrib/assets/149630/abea1b72-392b-4f1f-a403-644c4e356f3d)

I discovered that the resource key used for the calculated metrics was a
map hash of the resource attributes. This worked fine for some
instrumented services, however, other services include attributes like
its process id etc. Restarting one of these services would result in a
new hash and calculated resource metrics (in addition to the existing
ones).

This pull-request filters the resource attributes used to produce the
resource metrics key map hash. I am now able to restart services without
breaking my counters.

---------

Signed-off-by: Sean Porter <portertech@gmail.com>
Co-authored-by: Albert <26584478+albertteoh@users.noreply.github.com>
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Jan 10, 2024
@portertech @cparkins
[connector/spanmetrics] Added @portertech to codeowners (open-telemet…
614701a 
…ry#30401)

Added @portertech (me) to connector/spanmetrics codeowners.

Supporters: @djaglowski, @albertteoh 

Recent relevant PR:
open-telemetry#29711

Signed-off-by: Sean Porter <portertech@gmail.com>
@rotscher rotscher mentioned this pull request Jan 11, 2024
Open
@portertech portertech mentioned this pull request Jan 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@songy23 songy23 songy23 left review comments

@MovieStoreGuy MovieStoreGuy MovieStoreGuy approved these changes

@albertteoh albertteoh albertteoh approved these changes

@codeboten codeboten codeboten approved these changes

Assignees

@dashpole dashpole

Labels
connector/spanmetrics ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Milestone
v0.92.0
Development

Successfully merging this pull request may close these issues.
6 participants
@portertech @codeboten @songy23 @albertteoh @MovieStoreGuy @dashpole