Skip to content

Add support for the spanmetrics connector#4452

Merged
yurishkuro merged 12 commits intojaegertracing:mainfrom
albertteoh:4345-spanmetrics-connector-1
May 27, 2023
Merged

Add support for the spanmetrics connector#4452
yurishkuro merged 12 commits intojaegertracing:mainfrom
albertteoh:4345-spanmetrics-connector-1

Conversation

@albertteoh
Copy link
Copy Markdown
Contributor

@albertteoh albertteoh commented May 13, 2023

Which problem is this PR solving?

Resolves #4345

Short description of the changes

  • Adds support for spanmetrics connector via config parameters, while maintaining backwards compatibility with spanmetrics processor.
  • Some quality of life changes added to help contributors with making changes to the SPM feature.

Testing

  • Add test cases for connector case, and rely on existing tests to assert backwards compatibility.
  • Manual testing via docker-compose to ensure both the service-level and operation-level metrics are visible in the Monitor tab.

Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
@albertteoh albertteoh requested a review from a team as a code owner May 13, 2023 06:40
@albertteoh albertteoh requested a review from vprithvi May 13, 2023 06:40
Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
@codecov
Copy link
Copy Markdown

codecov bot commented May 13, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02 ⚠️

Comparison is base (015020d) 97.05% compared to head (5dab4f5) 97.04%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4452      +/-   ##
==========================================
- Coverage   97.05%   97.04%   -0.02%     
==========================================
  Files         300      300              
  Lines       17740    17813      +73     
==========================================
+ Hits        17217    17286      +69     
- Misses        419      422       +3     
- Partials      104      105       +1     
Flag Coverage Δ
unittests 97.04% <100.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
plugin/metrics/prometheus/factory.go 100.00% <100.00%> (ø)
...trics/prometheus/metricsstore/dbmodel/to_domain.go 100.00% <100.00%> (ø)
plugin/metrics/prometheus/metricsstore/reader.go 100.00% <100.00%> (ø)
plugin/metrics/prometheus/options.go 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
@albertteoh
Copy link
Copy Markdown
Contributor Author

@yurishkuro please let me know if there's anything else that needs addressing in this PR.

@yurishkuro yurishkuro enabled auto-merge (squash) May 27, 2023 18:03
@yurishkuro yurishkuro merged commit 9c787fc into jaegertracing:main May 27, 2023
@albertteoh albertteoh deleted the 4345-spanmetrics-connector-1 branch May 28, 2023 07:24
@hdong69
Copy link
Copy Markdown

hdong69 commented May 31, 2023

hi @albertteoh @yurishkuro. Are the changes included in all the images of Jaeger componets (agent, colector, query,....) version 1.45 ? If not, is any addional configuration needed to add ? Thanks

@albertteoh
Copy link
Copy Markdown
Contributor Author

Are the changes included in all the images of Jaeger componets (agent, colector, query,....) version 1.45 ?

@hdong69 this change should be available from the next release (version 1.46.0), and is only relevant to jaeger all-in-one and jaeger-query components.

is any addional configuration needed to add ?

Please find instructions here: https://github.com/jaegertracing/jaeger/tree/main/docker-compose/monitor#migrating

@hdong69
Copy link
Copy Markdown

hdong69 commented May 31, 2023

@albertteoh thanks for your feedback. so I understand that following the instruction (adding some variables in jaeger-query,...) could make the current version of jaeger query (jaegertracing/jaeger-query:1.45) works (curently, p95 doesnot appear in my Jaeger UI ) ? please correct me if i am wrong. thanks

@albertteoh
Copy link
Copy Markdown
Contributor Author

albertteoh commented May 31, 2023

so I understand that following the instruction (adding some variables in jaeger-query,...) could make the current version of jaeger query (jaegertracing/jaeger-query:1.45) works (curently, p95 doesnot appear in my Jaeger UI ) ?

Apologies, I wasn't clear. The instructions above only apply from version 1.46.

jaeger-query 1.45 will only work (for Latencies) with OTEL collector versions 0.70.0 and below. It was broken because of a change to the prometheusexporter in OTEL collector.

I'll create a PR that updates the v1.46 documentation to include these details.

@hdong69
Copy link
Copy Markdown

hdong69 commented May 31, 2023

so I understand that following the instruction (adding some variables in jaeger-query,...) could make the current version of jaeger query (jaegertracing/jaeger-query:1.45) works (curently, p95 doesnot appear in my Jaeger UI ) ?

Apologies, I wasn't clear. The instructions above only apply from version 1.46.

jaeger-query 1.45 will only work (for Latencies) with OTEL collector versions 0.70.0 and below. It was broken because of a breaking change to the prometheusexporter in OTEL collector.

I'll create a PR that updates the v1.46 documentation to include these details.

okay i got it. Many thanks

@tmc
Copy link
Copy Markdown

tmc commented Jun 4, 2023

Can we get a 1.46 release out?

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.

[Feature/SPM]: Support spanmetrics connector

4 participants