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

peer.service adjustment for sql propagation with DBM #3127

Merged
merged 7 commits into from
Sep 13, 2023
Merged

Conversation

zarirhamza
Copy link
Contributor

This PR addresses the issue of DBM relying on service_name for their own service names prior to peer.service. Now since all services are flattened, the DBM map is inaccurate and a better tag to create a more accurate map is to use peer.service

@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Sep 12, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2023

Codecov Report

Merging #3127 (33d1a13) into master (6ac6439) will increase coverage by 0.00%.
Report is 49 commits behind head on master.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3127   +/-   ##
=======================================
  Coverage   98.16%   98.16%           
=======================================
  Files        1323     1323           
  Lines       75292    75318   +26     
  Branches     3476     3476           
=======================================
+ Hits        73908    73934   +26     
  Misses       1384     1384           
Files Changed Coverage Δ
.../datadog/tracing/contrib/mysql2/instrumentation.rb 100.00% <100.00%> (ø)
...datadog/tracing/contrib/propagation/sql_comment.rb 100.00% <100.00%> (ø)
...og/tracing/contrib/propagation/sql_comment_spec.rb 100.00% <100.00%> (ø)
...racing/contrib/sql_comment_propagation_examples.rb 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zarirhamza zarirhamza marked this pull request as ready for review September 13, 2023 15:30
@zarirhamza zarirhamza requested a review from a team September 13, 2023 15:30
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

One minor nit.

@@ -102,6 +103,63 @@
end
end

context 'when tracing is enabled with peer.service' do
Copy link
Contributor

Choose a reason for hiding this comment

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

The description does not match the scenario, it only need a span_op with a specific tag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree, the main thing here is to mock a situation when tracing is enabled with peer.service, ensuring that the peer.service value is getting set as the dddbs tag.

it only need a span_op with a specific tag

That's what the peer.service feature does. It adds a tag

Copy link
Contributor

@TonyCTHsu TonyCTHsu Sep 13, 2023

Choose a reason for hiding this comment

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

Sorry, I meant that the test could be simplified, for example,

We don't need

        Datadog.configure do |c|
          c.env = 'production'
          c.service = "Traders' Joe"
          c.version = '1.0.0'
        end

Because, the method is stateless, only depends on the input.

Copy link
Contributor

Choose a reason for hiding this comment

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

The description should be

context 'when given a span operation tagged with`peer.service`' do

@zarirhamza zarirhamza merged commit 1c5860d into master Sep 13, 2023
175 checks passed
@zarirhamza zarirhamza deleted the zarir/ddbs_fix branch September 13, 2023 20:02
@github-actions github-actions bot added this to the 1.15.0 milestone Sep 13, 2023
ynnadkrap added a commit that referenced this pull request Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants