Skip to content

add recorder + scheduler#6

Merged
rochdev merged 2 commits into
masterfrom
recorder
Jan 31, 2018
Merged

add recorder + scheduler#6
rochdev merged 2 commits into
masterfrom
recorder

Conversation

@rochdev

@rochdev rochdev commented Jan 31, 2018

Copy link
Copy Markdown
Member

Adds the Recorder class which orchestrates the writer and handles flush delays, and the Scheduler class which abstracts the timers.

@rochdev
rochdev requested review from palazzem and vinvol January 31, 2018 20:28
Comment thread src/recorder.js Outdated
this._scheduler.start()
}

if (this._writer.length < this._tracer._bufferSize) {

@clutchski clutchski Jan 31, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i'd push all of this into the writer itself. it's slightly confusing that the recorder enforces the writers buffer size and resets schedulers, etc?

i think it'd just do (pseudo-code alert)

init() {
   this.scheduler = New Scheduler()
   this.scheduler.start(this.writer.flush)
}
record(trace) {
   this.writer.append(trace) // the writer can flush itself if it's full. i also woouldn't worry about resetting the timer 
}

Comment thread src/scheduler.js Outdated
// TODO: flush on process exit

class Scheduler {
constructor (callback, delay) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nitpick: s/delay/interval

Comment thread src/recorder.js Outdated
// TODO: make calls to Writer#append asynchronous

class Recorder {
constructor (tracer) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i think this class should just depend on url, interval and buffeer size. then it's clear it doesn't really need a tracer, just a few variables. looser coupling basically.

@clutchski

Copy link
Copy Markdown

made some small comments but all in all, looks good.

@rochdev
rochdev merged commit 2ab405d into master Jan 31, 2018
@rochdev
rochdev deleted the recorder branch January 31, 2018 21:29
@palazzem palazzem added the core label Feb 1, 2018
@palazzem palazzem added this to the 0.1.0 milestone Feb 1, 2018
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]>
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