Skip to content

add writer#3

Merged
rochdev merged 1 commit into
masterfrom
writer
Jan 29, 2018
Merged

add writer#3
rochdev merged 1 commit into
masterfrom
writer

Conversation

@rochdev

@rochdev rochdev commented Jan 26, 2018

Copy link
Copy Markdown
Member

Adds a Writer class that acts as the buffer between the tracer and the transport. It also handles encoding every trace to MsgPack. Since the writer also optimizes how traces are encoded as they are received, that's why it also handles encoding.

@rochdev
rochdev requested a review from palazzem January 26, 2018 23:13
@rochdev
rochdev force-pushed the writer branch 2 times, most recently from 97ceb96 to 2a277ce Compare January 26, 2018 23:46
@palazzem palazzem added the core label Jan 27, 2018
@palazzem palazzem added this to the 0.1.0 milestone Jan 27, 2018
@palazzem
palazzem requested a review from vinvol January 27, 2018 11:24
@rochdev
rochdev merged commit 011c6c3 into master Jan 29, 2018
@rochdev
rochdev deleted the writer branch January 29, 2018 18:25
alrosot pushed a commit to alrosot/dd-trace-js that referenced this pull request May 20, 2021
crysmags added a commit that referenced this pull request May 14, 2026
After merging master, #8309's perf wins target functions that either no
longer exist (wrapExecute / wrapFields / pathToArray / finishResolvers
moved out of instrumentations/graphql.js) or live in a deleted file
(resolve.js was folded into execute.js). Port the still-applicable
changes onto the migrated execute plugin:

  #1 _startResolveSpan uses info.fieldNodes[0] instead of .find(fn =>
     fn.kind === 'Field'). The executor only hands FieldNode siblings.
  #2 pathToArray switches to count-then-fill (drops the trailing
     .reverse() and the push-grow).
  #3 getResolverInfo lazy-allocates resolverVars: skips both the
     up-front {} and the directive-loop assignment when args is empty
     and no directives have arguments.
  #4 shouldInstrument early-returns on config.depth < 0 (no-limit, the
     default-enabled case) before walking the path.
  #5 buildCollapsedPathString builds the collapsed key in one walk;
     drops the intermediate collapsedPath array. iastResolveCh.publish
     now passes raw path / pathString, matching the contract the old
     resolve.js had with the IAST subscriber.
  drive-by  wrapFields iterates Object.values(type._fields) directly.

  #7 (addVariableTags early-return) already auto-merged during the
     master merge.
  #6 (single documentSources.get in wrapExecute) is already the shape
     of bindStart in the migrated plugin.

The reverted depth=0 bindStart short-circuit (156a557 / ca2d041)
sat at a different position — inside bindStart, gated on _depthDisabled
(config.depth === 0). It was rolled back because piling it on top of
the resolveAsync depth-flag fix didn't move plugin-graphql-long-with-
depth-off any further. This port stays inside shouldInstrument, fires
on config.depth < 0 (the *enabled* default), and targets the hot path
on plugin-graphql-long.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
crysmags added a commit that referenced this pull request May 19, 2026
After merging master, #8309's perf wins target functions that either no
longer exist (wrapExecute / wrapFields / pathToArray / finishResolvers
moved out of instrumentations/graphql.js) or live in a deleted file
(resolve.js was folded into execute.js). Port the still-applicable
changes onto the migrated execute plugin:

  #1 _startResolveSpan uses info.fieldNodes[0] instead of .find(fn =>
     fn.kind === 'Field'). The executor only hands FieldNode siblings.
  #2 pathToArray switches to count-then-fill (drops the trailing
     .reverse() and the push-grow).
  #3 getResolverInfo lazy-allocates resolverVars: skips both the
     up-front {} and the directive-loop assignment when args is empty
     and no directives have arguments.
  #4 shouldInstrument early-returns on config.depth < 0 (no-limit, the
     default-enabled case) before walking the path.
  #5 buildCollapsedPathString builds the collapsed key in one walk;
     drops the intermediate collapsedPath array. iastResolveCh.publish
     now passes raw path / pathString, matching the contract the old
     resolve.js had with the IAST subscriber.
  drive-by  wrapFields iterates Object.values(type._fields) directly.

  #7 (addVariableTags early-return) already auto-merged during the
     master merge.
  #6 (single documentSources.get in wrapExecute) is already the shape
     of bindStart in the migrated plugin.

The reverted depth=0 bindStart short-circuit (156a557 / ca2d041)
sat at a different position — inside bindStart, gated on _depthDisabled
(config.depth === 0). It was rolled back because piling it on top of
the resolveAsync depth-flag fix didn't move plugin-graphql-long-with-
depth-off any further. This port stays inside shouldInstrument, fires
on config.depth < 0 (the *enabled* default), and targets the hot path
on plugin-graphql-long.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
christophe-papazian added a commit that referenced this pull request Jun 16, 2026
- #3 (regression from round 5): constraintMatchesSlash no longer treats a '/' that
  appears only inside a character class as slash-spanning. `[^/]+` denies slashes, so
  /:id([^/]+)/users (Express 4) normalizes to /{id}/users again instead of null.
- #2: a non-terminal catch-all (a wildcard with a non-empty segment after it, incl. an
  optional {/*rest}/tail) is rejected at compile (→ null) rather than silently dropped.
- #1: a segment containing two independent optional groups (e.g. :a{.:b}{-:c}) is
  rejected (→ null); our single per-segment regex can't replicate path-to-regexp's
  ordered-alternative assignment, so the combined name could be wrong. Single
  intra-segment optionals (:id{.:format}) are unaffected.
- #4: structural-only optional groups — a {...} wrapping only nested group(s) with no
  segment/token of its own (e.g. the outer braces in /a{{/b}}) — are collapsed by
  reparenting represented groups to their nearest represented ancestor, so /a{{/b}}
  resolves to /a/b | /a instead of always /a.

Adds a regression test per finding (verified against live Express 5).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants