Skip to content

[Fix] Restore ES metrics #7006

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
Merged
merged 30 commits into from
May 12, 2025
Merged

[Fix] Restore ES metrics #7006

merged 30 commits into from
May 12, 2025

Conversation

AnmolxSingh
Copy link
Contributor

@AnmolxSingh AnmolxSingh commented Apr 9, 2025 •
edited
Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

  • Ran writer tests

Checklist

@AnmolxSingh
metrics introduced
f292c73 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
@AnmolxSingh
lint fix
64d185b 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
@AnmolxSingh AnmolxSingh marked this pull request as ready for review April 9, 2025 19:28
@AnmolxSingh AnmolxSingh requested a review from a team as a code owner April 9, 2025 19:28
@AnmolxSingh AnmolxSingh requested a review from jkowall April 9, 2025 19:28
@codecov Codecov
Copy link

codecov bot commented Apr 9, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 95.34884% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.20%. Comparing base (1e691d5) to head (6b6d8ea).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
internal/storage/elasticsearch/config/config.go 95.12% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7006      +/-   ##
==========================================
+ Coverage   96.12%   96.20%   +0.07%     
==========================================
  Files         357      358       +1     
  Lines       21570    21596      +26     
==========================================
+ Hits        20735    20777      +42     
+ Misses        629      613      -16     
  Partials      206      206
Flag Coverage Δ
badger_v1 9.90% <0.00%> (-0.03%) ⬇️
badger_v2 2.05% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v1-manual 14.89% <0.00%> (-0.04%) ⬇️
cassandra-4.x-v2-auto 2.04% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v2-manual 2.04% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v1-manual 14.89% <0.00%> (-0.04%) ⬇️
cassandra-5.x-v2-auto 2.04% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v2-manual 2.04% <0.00%> (-0.01%) ⬇️
elasticsearch-6.x-v1 20.23% <53.48%> (+0.03%) ⬆️
elasticsearch-7.x-v1 20.31% <53.48%> (+0.03%) ⬆️
elasticsearch-8.x-v1 20.49% <53.48%> (+0.03%) ⬆️
elasticsearch-8.x-v2 2.05% <0.00%> (-0.01%) ⬇️
grpc_v1 11.44% <0.00%> (-0.03%) ⬇️
grpc_v2 2.05% <0.00%> (-0.01%) ⬇️
kafka-3.x-v1 10.17% <0.00%> (-0.03%) ⬇️
kafka-3.x-v2 2.05% <0.00%> (-0.01%) ⬇️
memory_v2 2.05% <0.00%> (-0.01%) ⬇️
opensearch-1.x-v1 20.36% <53.48%> (+0.03%) ⬆️
opensearch-2.x-v1 20.36% <53.48%> (+0.03%) ⬆️
opensearch-2.x-v2 2.05% <0.00%> (-0.01%) ⬇️
query 2.05% <0.00%> (-0.01%) ⬇️
tailsampling-processor 0.55% <0.00%> (-0.01%) ⬇️
unittests 95.02% <83.72%> (+0.08%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@AnmolxSingh
Copy link
Contributor Author

@yurishkuro I just wanted to know about my direction at moment.

Manik2708
Manik2708 reviewed Apr 10, 2025
View reviewed changes
AnmolxSingh and others added 2 commits April 11, 2025 01:57
@AnmolxSingh
Merge branch 'main' into metrics
7e9e2ba
@AnmolxSingh
lint checks fixed
17756d8 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
yurishkuro
yurishkuro reviewed Apr 10, 2025
View reviewed changes
yurishkuro
yurishkuro reviewed Apr 10, 2025
View reviewed changes
yurishkuro
yurishkuro reviewed Apr 10, 2025
View reviewed changes
yurishkuro
yurishkuro reviewed Apr 10, 2025
View reviewed changes
@AnmolxSingh AnmolxSingh force-pushed the metrics branch 2 times, most recently from f5011af to 17756d8 Compare April 11, 2025 19:04
AnmolxSingh and others added 2 commits April 12, 2025 00:35
@AnmolxSingh
Merge branch 'main' into metrics
519a5f5
@AnmolxSingh
suggestions done
e2dfb3c 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
yurishkuro
yurishkuro reviewed Apr 11, 2025
View reviewed changes
AnmolxSingh and others added 2 commits April 11, 2025 20:23
@AnmolxSingh
indexcreate -> spanwrite
5acecc4 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
@AnmolxSingh @yurishkuro
Update internal/storage/elasticsearch/config/config.go
277cabb 
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Anmol <166167480+AnmolxSingh@users.noreply.github.com>
yurishkuro
yurishkuro reviewed Apr 12, 2025
View reviewed changes
AnmolxSingh and others added 2 commits April 14, 2025 01:12
@AnmolxSingh
Merge branch 'main' into metrics
9ff5576
@AnmolxSingh
reduntant code fixed
628f621 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
yurishkuro
yurishkuro reviewed Apr 13, 2025
View reviewed changes
yurishkuro
yurishkuro reviewed Apr 27, 2025
View reviewed changes
yurishkuro
yurishkuro reviewed Apr 27, 2025
View reviewed changes
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.

I don't see any unit tests validating that the expected metrics are being emitted

@yurishkuro
fix
a5b2ab4 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro
fmt
b768dfc 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@AnmolxSingh
Copy link
Contributor Author

I don't see any unit tests validating that the expected metrics are being emitted

While writing the unit tests for emitted metrices I was thinking of simulating the .After() callback.
Is this approach correct?

@yurishkuro
Copy link
Member

I was thinking of simulating the

you can move the code of the callback into a top level function which you can call directly from the test

AnmolxSingh
AnmolxSingh commented Apr 30, 2025
View reviewed changes
@AnmolxSingh
tests
b7733ac 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
yurishkuro
yurishkuro reviewed May 1, 2025
View reviewed changes
@AnmolxSingh
fix tests
1048941 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
AnmolxSingh
AnmolxSingh commented May 4, 2025
View reviewed changes
AnmolxSingh added 2 commits May 6, 2025 11:12
@AnmolxSingh
fix
2adbcd7 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
@AnmolxSingh
fix
3d1c711 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
yurishkuro
yurishkuro reviewed May 6, 2025
View reviewed changes
@AnmolxSingh
fix
e6b93d3 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
yurishkuro
yurishkuro reviewed May 9, 2025
View reviewed changes
@AnmolxSingh
fix
39ddd02 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
yurishkuro
yurishkuro reviewed May 11, 2025
View reviewed changes
yurishkuro
yurishkuro reviewed May 11, 2025
View reviewed changes
AnmolxSingh
AnmolxSingh commented May 11, 2025
View reviewed changes
@AnmolxSingh
fix
6b6d8ea 
Signed-off-by: anmol7344 <anmol7344@gmail.com>
@yurishkuro yurishkuro changed the title [Fix] ES metrics reintroduced [Fix] Restore ES metrics May 12, 2025
@yurishkuro yurishkuro added this pull request to the merge queue May 12, 2025
Merged via the queue into jaegertracing:main with commit 176aeed May 12, 2025
58 checks passed
iypetrov pushed a commit to iypetrov/jaeger that referenced this pull request May 14, 2025
@AnmolxSingh @yurishkuro @iypetrov
[Fix] Restore ES metrics (jaegertracing#7006)
3ccc1ea 
## Which problem is this PR solving?
-  Resolves jaegertracing#6891 

## Description of the changes
- Reintroduced metrics which were lost during jaegertracing#6883 

## How was this change tested?
- Ran writer tests

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

---------

Signed-off-by: anmol7344 <anmol7344@gmail.com>
Signed-off-by: Anmol <166167480+AnmolxSingh@users.noreply.github.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <github@ysh.us>
@AnmolxSingh AnmolxSingh deleted the metrics branch May 16, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@jkowall jkowall Awaiting requested review from jkowall jkowall is a code owner automatically assigned from jaegertracing/jaeger-maintainers

+1 more reviewer

@Manik2708 Manik2708 Manik2708 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
changelog:bugfix-or-minor-feature storage/elasticsearch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: ES store metrics got lost
3 participants
@AnmolxSingh @yurishkuro @Manik2708