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

Add support for the spanmetrics connector #4452

Merged
merged 12 commits into from
May 27, 2023

Conversation

albertteoh
Copy link
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
docker-compose/monitor/Makefile Show resolved Hide resolved
docker-compose/monitor/README.md Show resolved Hide resolved
docker-compose/monitor/docker-compose-processor.yml Outdated Show resolved Hide resolved
plugin/metrics/prometheus/factory.go Show resolved Hide resolved
plugin/metrics/prometheus/options.go Show resolved Hide resolved
plugin/metrics/prometheus/options.go Outdated Show resolved Hide resolved
Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
@codecov
Copy link

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.

docker-compose/monitor/README.md Show resolved Hide resolved
docker-compose/monitor/README.md Show resolved Hide resolved
docker-compose/monitor/README.md Outdated Show resolved Hide resolved
docker-compose/monitor/README.md Outdated Show resolved Hide resolved
docker-compose/monitor/docker-compose-connector.yml Outdated Show resolved Hide resolved
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
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

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
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

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
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

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

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