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

apm.startTransaction() should return a NoopTransaction if the Agent is not yet started #2429

Closed
trentm opened this issue Nov 8, 2021 · 3 comments
Assignees
Labels
agent-nodejs Make available for APM Agents project planning.
Milestone

Comments

@trentm
Copy link
Member

trentm commented Nov 8, 2021

See discussion on #2425. It was decided there that, while the TypeScript types in index.d.ts allow apm.startTransaction() returning null is some cases, it could be a breaking change to JS users of apm.startTransaction(...) that did not handle this case -- especially because the Agent API docs for startTransaction don't mention the possibility of getting null.

Update: This comment has the best description for why we are dropping | null from the return type of apm.startTransaction().

@trentm trentm added this to the next-major milestone Nov 8, 2021
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Nov 8, 2021
@trentm
Copy link
Member Author

trentm commented Jul 25, 2023

Consider changing this plan to instead return a NoopTransaction (i.e. taking advice from what OTel does in the equivalent case). And then could remove | null from index.d.ts for startTransaction().

@trentm trentm changed the title apm.createTransaction() should return null if the Agent is not yet started apm.createTransaction() should return a NoopTransaction if the Agent is not yet started Jul 25, 2023
@trentm
Copy link
Member Author

trentm commented Aug 3, 2023

taking advice from what OTel does in the equivalent case

OTel API has a NonRecordingSpan.

export const INVALID_SPANID = '0000000000000000';
export const INVALID_TRACEID = '00000000000000000000000000000000';
export const INVALID_SPAN_CONTEXT: SpanContext = {
  traceId: INVALID_TRACEID,
  spanId: INVALID_SPANID,
  traceFlags: TraceFlags.NONE,
};

Proposal:

  • NoopTransaction, uses all-zeros ids. Implements all of interface Transaction
  • Agent.protototype.startTrannsaction returns NoopTransaction if agent is not yet started.
  • remove | null from startTransaction type.
  • add a test case that exercises a transaction created via the API without the agent started.

@david-luna david-luna self-assigned this Aug 4, 2023
@david-luna david-luna changed the title apm.createTransaction() should return a NoopTransaction if the Agent is not yet started apm.startTransaction() should return a NoopTransaction if the Agent is not yet started Aug 4, 2023
trentm added a commit that referenced this issue Aug 4, 2023
…t started (#3554)

This also removes `| null` from the retval type of `.startTransaction()`.

Co-authored-by: Trent Mick <trent.mick@elastic.co>
Closes: #2429
@trentm
Copy link
Member Author

trentm commented Aug 4, 2023

Closed by #3554 on the "dev/4.x" branch, which will be the active 4.x branch soonish.

@trentm trentm closed this as completed Aug 4, 2023
trentm added a commit that referenced this issue Sep 1, 2023
feat!: the start of v4.x

Significant and breaking changes:
- Min supported Node.js is now 14.5.0 (up from 8.6).
- Drop support for the obsolete "patch" context manager, i.e. the
  `contextManager: "patch"` config option. This was a limited async context
  management that predated the preferred `AsyncLocalStorage` core Node.js
  mechanism for context tracking. (#3529)
- Config vars:
    - Remove `logUncaughtExceptions` config option, if the agent's
      `uncaughtException` handler is active it now *always* logs the error to
      the console. (#2412)
    - Remove `filterHttpHeaders` config option, see `sanitizeFieldNames`
      instead. (#3332)
    - Remove long deprecate `ELASTIC_APM_KUBERNETES_*` envvars, use
      `KUBERNETES_*` instead. (#2661)
    - The `useElasticTraceparentHeader` config option now defaults to `false`.
      This means the `elastic-apm-traceparent` HTTP header is now no longer
      sent by default. (#3555)
    - Drop erroneous `ELASTIC_SANITIZE_FIELD_NAMES` and
      `ELASTIC_IGNORE_MESSAGE_QUEUES` config envvars.
- Instrumentations:
    - Drop instrumentation for old `hapi`, the current `@hapi/hapi` is still
      instrumented. (#2691)
- APIs:
    - Ignore a `timer` option passed to `startTransaction()` and `startSpan()`
      APIs. (#2990)
    - Remove the deprecated `span.toString()` and `transaction.toString()` APIs.
      (#2348)
    - Change `apm.startTransaction()` API to return a noop transaction instead
      of null when the agent is not started. (#2429)
    - Remove `transaction.subtype` and `transaction.action` properties from the
      API. This also impacts <<apm-start-transaction>> and
      `transaction.setType(...)`, both of which now no longer accept `subtype`
      and `action` parameters. (#3557)
fpm-peter pushed a commit to fpm-git/apm-agent-nodejs that referenced this issue Aug 20, 2024
feat!: the start of v4.x

Significant and breaking changes:
- Min supported Node.js is now 14.5.0 (up from 8.6).
- Drop support for the obsolete "patch" context manager, i.e. the
  `contextManager: "patch"` config option. This was a limited async context
  management that predated the preferred `AsyncLocalStorage` core Node.js
  mechanism for context tracking. (elastic#3529)
- Config vars:
    - Remove `logUncaughtExceptions` config option, if the agent's
      `uncaughtException` handler is active it now *always* logs the error to
      the console. (elastic#2412)
    - Remove `filterHttpHeaders` config option, see `sanitizeFieldNames`
      instead. (elastic#3332)
    - Remove long deprecate `ELASTIC_APM_KUBERNETES_*` envvars, use
      `KUBERNETES_*` instead. (elastic#2661)
    - The `useElasticTraceparentHeader` config option now defaults to `false`.
      This means the `elastic-apm-traceparent` HTTP header is now no longer
      sent by default. (elastic#3555)
    - Drop erroneous `ELASTIC_SANITIZE_FIELD_NAMES` and
      `ELASTIC_IGNORE_MESSAGE_QUEUES` config envvars.
- Instrumentations:
    - Drop instrumentation for old `hapi`, the current `@hapi/hapi` is still
      instrumented. (elastic#2691)
- APIs:
    - Ignore a `timer` option passed to `startTransaction()` and `startSpan()`
      APIs. (elastic#2990)
    - Remove the deprecated `span.toString()` and `transaction.toString()` APIs.
      (elastic#2348)
    - Change `apm.startTransaction()` API to return a noop transaction instead
      of null when the agent is not started. (elastic#2429)
    - Remove `transaction.subtype` and `transaction.action` properties from the
      API. This also impacts <<apm-start-transaction>> and
      `transaction.setType(...)`, both of which now no longer accept `subtype`
      and `action` parameters. (elastic#3557)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

No branches or pull requests

2 participants