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.

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

Remove support for expvar metrics #5437

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

joeyyy09
Copy link

Which problem is this PR solving?

Description of the changes

  • This PR removes deprecated expvar CLI flags and the related backend from the codebase.

How was this change tested?

  • Tested locally by running the application without the deprecated flags to ensure that it functions correctly.

Checklist

Signed-off-by: Joeyyy09 <imt_2021063@iiitm.ac.in>
@yurishkuro
Copy link
Member

I would start with deleting internal/metrics/expvar and then making sure make build-binaries-(your platform) is ok.

examples/hotrod/cmd/flags.go Show resolved Hide resolved
examples/hotrod/cmd/flags.go Outdated Show resolved Hide resolved
examples/hotrod/cmd/root.go Outdated Show resolved Hide resolved
internal/metrics/metricsbuilder/builder.go Outdated Show resolved Hide resolved
joeyyy09 and others added 2 commits May 11, 2024 00:26
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Harshith Mente <109957201+joeyyy09@users.noreply.github.com>
Signed-off-by: Joeyyy09 <imt_2021063@iiitm.ac.in>
@joeyyy09
Copy link
Author

I would start with deleting internal/metrics/expvar and then making sure make build-binaries-(your platform) is ok.

Yeah, i removed them and made sure the build is okay.

@joeyyy09
Copy link
Author

@yurishkuro can you review these changes and let me know if i've got to change any more?

Copy link

codecov bot commented May 11, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 95.27%. Comparing base (5ddcaa9) to head (1715c01).
Report is 1 commits behind head on main.

Files Patch % Lines
internal/metrics/metricsbuilder/builder.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5437      +/-   ##
==========================================
- Coverage   95.29%   95.27%   -0.03%     
==========================================
  Files         334      331       -3     
  Lines       16233    16113     -120     
==========================================
- Hits        15469    15351     -118     
+ Misses        583      582       -1     
+ Partials      181      180       -1     
Flag Coverage Δ
badger_v1 7.97% <ø> (ø)
badger_v2 1.88% <ø> (ø)
cassandra-3.x-v1 16.27% <ø> (ø)
cassandra-3.x-v2 1.81% <ø> (ø)
cassandra-4.x-v1 16.27% <ø> (ø)
cassandra-4.x-v2 1.81% <ø> (ø)
elasticsearch-5.x 1.73% <ø> (ø)
elasticsearch-6.x 1.73% <ø> (+0.01%) ⬆️
elasticsearch-7.x 1.72% <ø> (-0.02%) ⬇️
elasticsearch-8.x 1.72% <ø> (-0.02%) ⬇️
grpc_v1 10.13% <ø> (ø)
grpc_v2 7.36% <ø> (+0.01%) ⬆️
kafka 9.68% <ø> (ø)
opensearch-1.x 1.73% <ø> (+0.01%) ⬆️
opensearch-2.x 1.72% <ø> (ø)
unittests 93.46% <50.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yurishkuro
Copy link
Member

please make sure make lint test are green

Signed-off-by: Joeyyy09 <imt_2021063@iiitm.ac.in>
@joeyyy09
Copy link
Author

please make sure make lint test are green

image

Yeah, fixed them. Thanks for the help! Is there anything i need to change?

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

You need to query Prometheus endpoint on each binary before and after changes to make sure no changes to metrics names happen.

cmd/all-in-one/main.go Outdated Show resolved Hide resolved
cmd/agent/main.go Outdated Show resolved Hide resolved
cmd/collector/main.go Outdated Show resolved Hide resolved
cmd/agent/main.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
internal/metrics/metricsbuilder/builder.go Outdated Show resolved Hide resolved
Signed-off-by: Joeyyy09 <imt_2021063@iiitm.ac.in>
@joeyyy09
Copy link
Author

You need to query Prometheus endpoint on each binary before and after changes to make sure no changes to metrics names happen.

Yeah, I'll verify this once.

@joeyyy09
Copy link
Author

joeyyy09 commented May 13, 2024

@yurishkuro I've resolved all the edits, thank you for helping me out. Absolutely sorry for messing up the namespaces. Can you please let me know if there's anything more i need to do? I'm willing to make the entire changes till this issue gets resolved.

@yurishkuro
Copy link
Member

I still see breaking changes. You need to test that the metrics names are the same, by running each of the main Jaeger components from your branch and from main (e.g. SPAN_STORAGE_TYPE=grpc-plugin go run ./cmd/query) and comparing the output of curl http://localhost:16687/metrics (admin port will be different for different binaries, see logs)

@joeyyy09
Copy link
Author

I still see breaking changes. You need to test that the metrics names are the same, by running each of the main Jaeger components from your branch and from main (e.g. SPAN_STORAGE_TYPE=grpc-plugin go run ./cmd/query) and comparing the output of curl http://localhost:16687/metrics (admin port will be different for different binaries, see logs)

Okay, will work on it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[deprecation] Remove support for expvar metrics
2 participants