Peer Service Precursors for DBM #2664
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2664 +/- ##
============================================
+ Coverage 77.66% 79.23% +1.56%
- Complexity 2210 2213 +3
============================================
Files 225 199 -26
Lines 26080 22078 -4002
Branches 988 0 -988
============================================
- Hits 20256 17494 -2762
+ Misses 5298 4584 -714
+ Partials 526 0 -526
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 26 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| if ($targetHost != "") { | ||
| $tags[Tag::DBM_PEER_HOSTNAME] = $targetHost; | ||
| } |
There was a problem hiding this comment.
This is not visible anymore given that you put these as constants, but this is "ddh", which is not respecting the order (before is "dddbs", next is "dddb"), see the // Note a few lines above.
Hence, to make this much more visible, I would recommend not using Tag:: here, but just adding it as a comment, e.g.
// peer hostname
$tags["ddh"] = $targetHost;
The strength of constants is when they are reused, but here's no reuse at all, so I think the approach with the comment would be better.
There was a problem hiding this comment.
which is not respecting the order
Wasn't aware there was an order
There was a problem hiding this comment.
Reference regarding ordering: https://google.github.io/sqlcommenter/spec/#sorting
There was a problem hiding this comment.
I'll make the changes
There was a problem hiding this comment.
It may also make sense to have a test actually verify these are ordered. At least you missed the comment in the source file :-D Others might too.
PROFeNoM
force-pushed
the
alex/dbm/ddprs-dddbs
branch
from
ca3d7fa
to
24bb876
Compare
| $query = self::propagateViaSqlComments($hook->args[$argNum], $databaseService, $propagationMode); | ||
| $targetHost = $span->meta[Tag::TARGET_HOST] ?? ''; | ||
| $dbName = $span->meta[Tag::DB_NAME] ?? $span->meta[Tag::DB_INSTANCE] ?? ''; | ||
| $peerService = $span->meta['peer.service'] ?? ''; |
There was a problem hiding this comment.
nit: this seems like the only tag not specified as Tag::... and I'm not sure if that's intentional or not
There was a problem hiding this comment.
Yes, this was sort of intentional since there is no Tag::PEER_SERVICE. I can add it - won't make any harm
Co-authored-by: Bob Weinand <bob.weinand@datadoghq.com>
Description
Please take a look at AIT-10031 for the full details (contains the short RFC).
Changed
dddbs: It should NOT be overridden by peer.service. There is some fake service thingy in the RFC, but I don't believe it applies there? Or there is something that I'm unaware ofAdded
ddh: The hostname of the database serverdddb: The schema/database/namespaceddprs: Value of peer.service if and only if the customer explicitly sets thisReviewer checklist