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

[V2]Add Script for metrics markdown table #5941

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

Conversation

vvs-personalstash
Copy link
Contributor

@vvs-personalstash vvs-personalstash commented Sep 5, 2024

Description of the changes

  • Added a script for creating a markdown table to view and compare v1 and v2 metrics using json generated by compare metrics

Checklist

Signed-off-by: vvs-personalstash <viralkverma0987@gmail.com>
Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.48%. Comparing base (08ae568) to head (308e152).
Report is 89 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5941      +/-   ##
==========================================
+ Coverage   91.04%   96.48%   +5.43%     
==========================================
  Files         345      352       +7     
  Lines       18062    19973    +1911     
==========================================
+ Hits        16445    19270    +2825     
+ Misses       1430      520     -910     
+ Partials      187      183       -4     
Flag Coverage Δ
badger_v1 8.42% <ø> (+0.39%) ⬆️
badger_v2 1.70% <ø> (-0.13%) ⬇️
cassandra-3.x-v1 ?
cassandra-3.x-v2 ?
cassandra-4.x-v1 14.57% <ø> (-0.89%) ⬇️
cassandra-4.x-v2 1.64% <ø> (-0.11%) ⬇️
cassandra-5.x-v1 14.57% <ø> (?)
cassandra-5.x-v2 1.64% <ø> (?)
elasticsearch-6.x-v1 18.73% <ø> (-0.06%) ⬇️
elasticsearch-7.x-v1 18.80% <ø> (-0.04%) ⬇️
elasticsearch-8.x-v1 18.98% <ø> (-0.05%) ⬇️
elasticsearch-8.x-v2 1.70% <ø> (-0.13%) ⬇️
grpc_v1 8.77% <ø> (-0.16%) ⬇️
grpc_v2 6.72% <ø> (-0.44%) ⬇️
kafka-v1 8.99% <ø> (-0.75%) ⬇️
kafka-v2 1.70% <ø> (-0.13%) ⬇️
memory_v2 1.70% <ø> (-0.13%) ⬇️
opensearch-1.x-v1 18.86% <ø> (+0.30%) ⬆️
opensearch-2.x-v1 18.86% <ø> (-0.04%) ⬇️
opensearch-2.x-v2 ?
tailsampling-processor 0.48% <ø> (+0.02%) ⬆️
unittests 95.39% <ø> (+0.10%) ⬆️

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.

@Wise-Wizard
Copy link
Contributor

In terminal, run make fmt and push again.

@@ -0,0 +1,101 @@
import json
Copy link
Member

Choose a reason for hiding this comment

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

  • move this into script/utils
  • please document how to use that
  • include generated MD file(s) - you can put then under cmd/jaeger/docs/migration

Signed-off-by: vvs-personalstash <viralkverma0987@gmail.com>

| Metric Name | Inner Parameters |
|-------------|------------------|
| go_gc_duration_seconds | |
Copy link
Member

Choose a reason for hiding this comment

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

@Wise-Wizard we should investigate how these standard Go metrics are being generated in v1 and decide how we can enable them in v2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, will look in that

Copy link
Member

Choose a reason for hiding this comment

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

this is what we might want: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/runtime/runtime.go

ideally Collector would expose those automatically, but it's not implemented currently -- open-telemetry/opentelemetry-collector#2155

@@ -0,0 +1,92 @@
### Common Metrics

| Metric Name | Inner Parameters |
Copy link
Member

Choose a reason for hiding this comment

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

Inner Parameters

The official term in Prometheus is Labels

print('Dict successfully converted to md')

# Usage
fn = '' #Enter the path of the JSON file generated by compare_metrics.py
Copy link
Member

@yurishkuro yurishkuro Sep 7, 2024

Choose a reason for hiding this comment

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

  1. let's move compare_metrics.py to utils too.
  2. could we just add the logic here to compare_metrics.py ? Maybe as an optional parameter --out=json|md

Copy link
Member

Choose a reason for hiding this comment

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

  1. I still don't see instructions how to run these things - the compare_metrics.py seems to hardcode the input file names like ./V1_Metrics.json -- please document how those JSON files should be produced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok should i create a separate documentation md file for this in scripts/utils or should i just add the instruction as comments

Copy link
Member

Choose a reason for hiding this comment

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

in comments is fine, this isn't something we'd use often

| processor_batch_timeout_trigger_send | processor, service_instance_id, service_name, service_version |
| receiver_accepted_spans | receiver, service_instance_id, service_name, service_version, transport |
| receiver_refused_spans | receiver, service_instance_id, service_name, service_version, transport |
| target_info | service_instance_id, service_name, service_version |
Copy link
Member

Choose a reason for hiding this comment

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

arguably this is equivalent to jaeger_build_info


| Metric Name | Inner Parameters |
|-------------|------------------|
| jaeger_badger_compaction_current_num_lsm | |
Copy link
Member

Choose a reason for hiding this comment

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

the storage-specific files will have large overlap with all-in-one, I recommend filtering out only the storage-related metrics.

@@ -0,0 +1,92 @@
### Common Metrics
Copy link
Member

Choose a reason for hiding this comment

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

could we just have a single table with a format similar to ### Equivalent Metrics? When one side is missing, put n/a

@yurishkuro yurishkuro added the changelog:exprimental Change to an experimental part of the code label Sep 8, 2024
Signed-off-by: vvs-personalstash <viralkverma0987@gmail.com>
Signed-off-by: vvs-personalstash <viralkverma0987@gmail.com>
@vvs-personalstash vvs-personalstash marked this pull request as ready for review October 24, 2024 07:54
@vvs-personalstash vvs-personalstash requested a review from a team as a code owner October 24, 2024 07:54
@dosubot dosubot bot added the v2 label Oct 24, 2024
@vvs-personalstash
Copy link
Contributor Author

@yurishkuro I have made changes as per your reviews,kindly review if there are any changes or errors i may have not noticed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:exprimental Change to an experimental part of the code v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants