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

[fix][spm]: Change default metrics namespace to match new default in spanmetricsconnector #6007

Merged
merged 13 commits into from
Sep 22, 2024

Conversation

mahadzaryab1
Copy link
Contributor

@mahadzaryab1 mahadzaryab1 commented Sep 21, 2024

Which problem is this PR solving?

Description of the changes

  • Make traces_span_metrics the new namespace default.
  • Fixed the failing SPM integration test by setting the default namespace when querying from prometheus to traces_span_metrics. This matches the new default namespace in the spanmetricsconnector which was changed in v0.109.0 (see f7o/opentelemetry-collector-contrib@371bc59)
  • This change is backwards compatible because it modifies the default. As a result, if anyone isn't using a custom namespace they will not be broken by these changes.

How was this change tested?

  • CI

Checklist

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Copy link

codecov bot commented Sep 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.79%. Comparing base (ff1b678) to head (cd5c713).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6007      +/-   ##
==========================================
+ Coverage   96.78%   96.79%   +0.01%     
==========================================
  Files         348      348              
  Lines       16559    16559              
==========================================
+ Hits        16027    16029       +2     
+ Misses        343      342       -1     
+ Partials      189      188       -1     
Flag Coverage Δ
badger_v1 8.02% <ø> (ø)
badger_v2 1.82% <ø> (ø)
cassandra-4.x-v1 16.61% <ø> (ø)
cassandra-4.x-v2 1.75% <ø> (ø)
cassandra-5.x-v1 16.61% <ø> (ø)
cassandra-5.x-v2 1.75% <ø> (ø)
elasticsearch-6.x-v1 18.77% <ø> (-0.02%) ⬇️
elasticsearch-7.x-v1 18.83% <ø> (ø)
elasticsearch-8.x-v1 19.04% <ø> (ø)
elasticsearch-8.x-v2 1.82% <ø> (+0.01%) ⬆️
grpc_v1 9.51% <ø> (-0.02%) ⬇️
grpc_v2 7.15% <ø> (ø)
kafka-v1 9.74% <ø> (ø)
kafka-v2 1.82% <ø> (ø)
memory_v2 1.82% <ø> (ø)
opensearch-1.x-v1 18.88% <ø> (ø)
opensearch-2.x-v1 18.89% <ø> (ø)
opensearch-2.x-v2 1.82% <ø> (ø)
tailsampling-processor 0.46% <ø> (ø)
unittests 95.28% <ø> (+0.01%) ⬆️

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.

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@@ -33,7 +33,7 @@ const (
defaultTokenFilePath = ""

defaultSupportSpanmetricsConnector = true
defaultMetricNamespace = ""
defaultMetricNamespace = "traces_span_metrics"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yurishkuro setting this default here seems to take care of the v2 test but the v1 test is still getting an empty response. do you have any idea why that could be? both v1 and v2 read this config.

Copy link
Contributor Author

@mahadzaryab1 mahadzaryab1 Sep 21, 2024

Choose a reason for hiding this comment

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

this was actually not related to the changes in this PR and was failing due to missing http receiver endpoint in the otel collector config but wasn't failing before because the version was pinned to 0.89.0

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@mahadzaryab1 mahadzaryab1 marked this pull request as ready for review September 21, 2024 21:56
@mahadzaryab1 mahadzaryab1 requested a review from a team as a code owner September 21, 2024 21:56
@dosubot dosubot bot added area/otel bug docker Pull requests that update Docker code labels Sep 21, 2024
@mahadzaryab1 mahadzaryab1 changed the title [WIP] fix(spm): incorporate breaking changes from otel collector default namespace update fix(spm): incorporate breaking changes from otel collector default namespace update Sep 21, 2024
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@mahadzaryab1 mahadzaryab1 changed the title fix(spm): incorporate breaking changes from otel collector default namespace update fix(spm): change default metrics namespace for prometheus to match new default namespace in spanmetricsconnector Sep 22, 2024
@yurishkuro yurishkuro added the changelog:breaking-change Change that is breaking public APIs or established behavior label Sep 22, 2024
@yurishkuro yurishkuro changed the title fix(spm): change default metrics namespace for prometheus to match new default namespace in spanmetricsconnector [fix][spm]: Change default metrics namespace for prometheus to match new default namespace in spanmetricsconnector Sep 22, 2024
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@mahadzaryab1 mahadzaryab1 changed the title [fix][spm]: Change default metrics namespace for prometheus to match new default namespace in spanmetricsconnector [fix][spm]: Change Default Metrics Namespace for Prometheus to Match New Default Namespace in spanmetricsconnector Sep 22, 2024
@yurishkuro yurishkuro changed the title [fix][spm]: Change Default Metrics Namespace for Prometheus to Match New Default Namespace in spanmetricsconnector [fix][spm]: Change default metrics namespace to match new default in spanmetricsconnector Sep 22, 2024
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.

Thanks for digging into this one! 🥳

@yurishkuro yurishkuro merged commit 6a96e18 into jaegertracing:main Sep 22, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/otel bug changelog:breaking-change Change that is breaking public APIs or established behavior docker Pull requests that update Docker code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SPM is broken by OTEL's new version
2 participants