Skip to content

ref(tracing): Make metadata property on Transaction class public #3557

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 20, 2021
Merged

ref(tracing): Make metadata property on Transaction class public #3557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
727fd19
add metadata to transaction type
lobsterkatie May 18, 2021
d13d48c
make metadata public on transaction class
lobsterkatie May 18, 2021
bf75a3d
export transaction metadata type
lobsterkatie May 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions packages/tracing/src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
import { getCurrentHub, Hub } from '@sentry/hub';
import { Event, Measurements, Transaction as TransactionInterface, TransactionContext } from '@sentry/types';
import {
Event,
Measurements,
Transaction as TransactionInterface,
TransactionContext,
TransactionMetadata,
} from '@sentry/types';
import { dropUndefinedKeys, isInstanceOf, logger } from '@sentry/utils';

import { Span as SpanClass, SpanRecorder } from './span';

interface TransactionMetadata {
transactionSampling?: { [key: string]: string | number };
}

/** JSDoc */
export class Transaction extends SpanClass implements TransactionInterface {
public name: string;

private _metadata: TransactionMetadata = {};
public metadata: TransactionMetadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to the previous comment. If we already have a setter, do we want to make it public? IMO it seems unnecessary and misleading. I'd either go with the getter/setter or the public attribute.

Copy link
Contributor

Choose a reason for hiding this comment

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

The setter above as you mentioned "merges", so it's helpful method. But I do agree that we could skip it if we'd go with get/set only, and not merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

It feels way simpler to me to just have it public. Honestly, IDK why I made it private to begin with.

One thing we might think about (in the long list of things we're thinking about for v7) is having a more coherent strategy for what's public and what's private with setters and getters. I don't feel like I have a great intuitive sense for which is better in cases like this.


private _measurements: Measurements = {};

Expand All @@ -39,6 +41,7 @@ export class Transaction extends SpanClass implements TransactionInterface {

this.name = transactionContext.name || '';

this.metadata = transactionContext.metadata || {};
this._trimEnd = transactionContext.trimEnd;

// this is because transactions are also spans, and spans have a transaction pointer
Expand Down Expand Up @@ -76,7 +79,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
* @hidden
*/
public setMetadata(newMetadata: TransactionMetadata): void {
this._metadata = { ...this._metadata, ...newMetadata };
this.metadata = { ...this.metadata, ...newMetadata };
}

/**
Expand Down Expand Up @@ -123,7 +126,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
timestamp: this.endTimestamp,
transaction: this.name,
type: 'transaction',
debug_meta: this._metadata,
debug_meta: this.metadata,
};

const hasMeasurements = Object.keys(this._measurements).length > 0;
Expand Down
1 change: 1 addition & 0 deletions packages/types/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export {
TraceparentData,
Transaction,
TransactionContext,
TransactionMetadata,
TransactionSamplingMethod,
} from './transaction';
export { Thread } from './thread';
Expand Down
20 changes: 20 additions & 0 deletions packages/types/src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ export interface TransactionContext extends SpanContext {
* If this transaction has a parent, the parent's sampling decision
*/
parentSampled?: boolean;

/**
* Metadata associated with the transaction, for internal SDK use.
*/
metadata?: TransactionMetadata;
}

/**
Expand Down Expand Up @@ -58,6 +63,11 @@ export interface Transaction extends TransactionContext, Span {
*/
data: { [key: string]: any };

/**
* Metadata about the transaction
*/
metadata: TransactionMetadata;

/**
* Set the name of the transaction
*/
Expand Down Expand Up @@ -113,3 +123,13 @@ export enum TransactionSamplingMethod {
Rate = 'client_rate',
Inheritance = 'inheritance',
}

export interface TransactionMetadata {
transactionSampling?: { rate?: number; method?: string };

/** The two halves (sentry and third-party) of a transaction's tracestate header, used for dynamic sampling */
tracestate?: {
sentry?: string;
thirdparty?: string;
};
}