Mar kolya/components suggestions - #580
Merged
Merged
Conversation
tylerbenson
reviewed
Nov 15, 2018
Contributor
There was a problem hiding this comment.
you're right... it doesn't make sense for this case. perhaps we should limit this error span somehow to http only?
Contributor
Author
There was a problem hiding this comment.
My impression is that http case doesn't even hit this code since we instrument http stuff separately...
tylerbenson
reviewed
Nov 15, 2018
Contributor
There was a problem hiding this comment.
Oops... I missed that it was already being set. Nice catch.
tylerbenson
approved these changes
Nov 15, 2018
Contributor
|
looks like there are format violations. fix that and merge it in. |
mar-kolya
force-pushed
the
mar-kolya/components-suggestions
branch
from
November 15, 2018 18:23
318ef63 to
465e237
Compare
jordan-wong
added a commit
that referenced
this pull request
Aug 20, 2026
…ferences (review #12114) Per architectural rule: the skill must be usable standalone; dependency direction is one-way (toolkit references the skill, never the reverse). The prior wording pointed the reader at the apm-instrumentation-toolkit (categories/database.md, PR #580, "when driven by the toolkit … force-read prompt blocks"). Removed all of it — the database-client design rules now stand on their own with no toolkit dependency. Also dropped the (R-DB-3) eval-bookkeeping tag, which is meaningless to a standalone skill reader. Rule content unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Aug 25, 2026
skill(apm-integrations): sharpen SPI + muzzle rules for database category, add eager-connect idiom From the database eval cycle (reference PRs #11996/#11997/#12032). Most database findings turned out to be adherence gaps against rules that already exist, not missing rules — so this sharpens the existing rules with the concrete failure modes, plus adds one genuinely-new idiom. instrumenter-module.md: - SPI rule: the ForTypeHierarchy exception now explicitly covers being handed a CONCRETE driver that implements a JDK SPI (e.g. org.postgresql.jdbc.PgStatement implements java.sql.Statement), not just interface-only spec jars. The old wording only triggered on "interface-only jar", so an agent given a single concrete driver didn't apply it — the PostgreSQL regen (R-DB-2) fell into exactly this trap and shipped a concrete-class module that also collides at runtime with the existing jdbc/ SPI module. - New: database clients must populate connection metadata eagerly at connect/factory time (JDBC DriverInstrumentation; R2DBC ConnectionFactoryOptions), not lazily per query (R-DB-3). muzzle.md: - assertInverse rule reinforced with the concrete-driver failure mode (R-DB-4): a pinned dependency version is not an API-shape boundary. Draft — will be refined as feedback comes in from the database reference PR reviews. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> skill(apm-integrations): couple module placement to a taken super() name (R-DB-1) Database category gap sweep (2026-08-05) confirmed R-DB-1 is category-wide, not a Cassandra quirk: any library with a version-sibling family directory whose name differs from the integration slug will collide. The existing "modify in place, don't create a parallel module" rule (line 47) doesn't cover the case the Cassandra regen actually hit: - eval slug `cassandra` != family dir `datastax-cassandra/` - surviving siblings (datastax-cassandra-3.0/-3.8) already declare super("cassandra") - under the blind protocol the same-version (4.0) module was DELETED, so "modify it in place" had no target — but the name was still taken The agent created a new top-level instrumentation/cassandra/ module with a duplicate super("cassandra") registration -> silent tracing outage (advice never applied, zero spans, tests timed out, no build error). Fix: grep the tree for the intended super() name BEFORE creating a module; if any module (including untouched version-siblings) holds it, join that family directory rather than minting a new top-level slug. Placement and name are one decision: a taken name dictates the directory. If there is no collision-free home, STOP and surface it. Verified against #12114's existing commit (37e661c): the SPI-collision case (R-DB-2) and eager-connect (R-DB-3) are already covered there; this is the distinct version-sibling-family placement case they don't address. Other sweep findings routed elsewhere (N-DBM-2 silent-downscope -> toolkit prompt; reviewer-check candidates -> toolkit repo), not this skill PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): cross-ref DB-client design rules to category guide Points instrumenter-module.md at the toolkit categories/database.md decision block (force-read at target-selection) for db.instance sourcing, wrapper pattern, DBM gating, and eager connect metadata (R-DB-3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): fix dangling toolkit cross-ref + retain lazy DBInfo fallback (review #12114) Addresses @dougqh's review: the DB-client design-rules bullet pointed at the toolkit file `categories/database.md` "force-read at target-selection" — but that file is not in this repo and "target-selection" is not a named SKILL.md step, so an agent reading the skill hit a dangling pointer. Reworded to: - name the real steps where the rules apply (Step 3 target selection / Step 5 module write); - state plainly that these are the human-readable rules, additionally enforced as force-read toolkit prompt blocks (apm-instrumentation-toolkit#580) when driven by the toolkit — and that the toolkit file is not part of this repo; - fold in the Codex/master-accurate note to retain a lazy parseDBInfo fallback for connect paths the eager Driver.connect hook doesn't cover (DataSource/proxy), not eager-only. No rule content removed; wording only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): fold 3 bot-review refinements — family-name evidence, concrete-hook scope, R2DBC create() (review #12114) Addresses valid Codex/Datadog-Autotest findings on #12114 (all confirmed against repo facts): - R-DB-1: "taken super() name dictates the directory" was too absolute. Reworded to treat name matches as EVIDENCE, then confirm by coordinates/packages/muzzle. Shared config names (jax-rs across rs/jersey/resteasy; ci-visibility across nine) do NOT identify one family → place by target library; if ambiguous, STOP. Also corrected the mechanism note: equal super() names do not themselves cause a registration outage (InstrumenterIndex indexes by module class); the real harm is a duplicate same-version module (the Cassandra case, unchanged). - R-DB-2: scoped the concrete-driver prohibition to BEHAVIORALLY-REDUNDANT advice. Vendor-only lifecycle/compat hooks not declared on the SPI legitimately need a concrete module (DBMCompatibleConnectionInstrumentation, DB2 JDBC, Tomcat Request.recycle()). Reject a concrete hook only when its method is already advised via the SPI. - R2DBC: corrected the factual claim that ConnectionFactoryOptions is available at ConnectionFactory.create(). create() is zero-arg returning a Publisher; options must be captured earlier at ConnectionFactories.get(options) into a ContextStore<ConnectionFactory,Options> and threaded onto the async-emitted Connection. Wording/accuracy only; no gating rule reversed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): R-DB-5 must keep the connection/session default, override only when richer (review #12114) Addresses Datadog Autotest :23 (valid): "derive db.instance from the operation not the connection default" read as "drop the default", which would lose db.instance/keyspace for operations with no result metadata (e.g. a Cassandra write). Reworded to the correct two-phase framing — keep the connection/session value as the DEFAULT, override per-operation only when the operation supplies a more specific value (fully-qualified other_ks.table, or keyspace from response ColumnDefinitions). This matches the toolkit R-DB-5 rule (session default + onResponse override), which was already correct; only the skill one-liner was imprecise. The other three Autotest :12/:59/:121 findings this run are re-scans already fixed by the prior two commits (concrete-hook scoping, name-as-evidence, R2DBC create() zero-arg). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): broaden super() name search + fix Cassandra outage causal claim (review #12114) Two valid Datadog Autotest catches (15:07 run): - :52 — the name-search grep matched only the FIRST super(...) arg, missing later args (super("vertx","vertx-sql-client")) and names held in constants. Broadened to grep all super(...) args and to also scan module classes / name constants when a literal grep misses. - :59 — the Cassandra "Concrete failure" paragraph attributed the tracing outage to two same-name @autoservice registrations, but modules are indexed by class name (equal names are legal). Reframed: the mechanism is a DUPLICATE module advising the same types (mutual suppression via the shared call-depth guard); the shared super("cassandra") name is a symptom, not the cause. Now consistent with the note added earlier. The third finding this run (:23 db.instance keep-the-default) is a stale re-scan already fixed in 04bfe52. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): make DB rules standalone — remove toolkit references (review #12114) Per architectural rule: the skill must be usable standalone; dependency direction is one-way (toolkit references the skill, never the reverse). The prior wording pointed the reader at the apm-instrumentation-toolkit (categories/database.md, PR #580, "when driven by the toolkit … force-read prompt blocks"). Removed all of it — the database-client design rules now stand on their own with no toolkit dependency. Also dropped the (R-DB-3) eval-bookkeeping tag, which is meaningless to a standalone skill reader. Rule content unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Merge branch 'master' into skill/database-category-rules-20260730 skill(apm-integrations): muzzle result is not proof a matcher target exists (review #12114) Addresses Codex P2 on muzzle.md: the prior text implied a muzzle pass verified the concrete matcher target (PgStatement) exists on old versions. Muzzle derives references from advice bytecode + explicit additional references, NOT from instrumentedType()/named(...) matcher strings, so a matcher-only concrete class is a blind spot. Reworded to: do not infer matcher-target presence/absence from a muzzle result; back version-dependent matcher rules with an explicit muzzle reference or a runtime/latest-dep test. Also folded in the PostgreSQL jdbc2/3/4 vs PgPreparedStatement nuance and dropped the imprecise "unchanged back through 9.2" claim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): make the family-placement example repo-generic (standalone) The skill must be readable by any agent working on dd-trace-java integrations, independent of the eval toolkit. Reworded the Cassandra family-placement example to drop eval-specific framing ("regen", "R-DB-1", "the eval was given the slug", "blind protocol") and state it as a plain worked example. Same lesson: a name taken by an existing family means join that family, not create a parallel module; the harm is duplicate advice on the same types. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.