Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions common/api-review/telemetry-angular.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export interface Telemetry {

// @public
export interface TelemetryOptions {
appVersion?: string;
endpointUrl?: string;
}

Expand Down
1 change: 1 addition & 0 deletions common/api-review/telemetry-react.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export interface Telemetry {

// @public
export interface TelemetryOptions {
appVersion?: string;
endpointUrl?: string;
}

Expand Down
1 change: 1 addition & 0 deletions common/api-review/telemetry.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export interface Telemetry {

// @public
export interface TelemetryOptions {
appVersion?: string;
endpointUrl?: string;
}

Expand Down
11 changes: 11 additions & 0 deletions docs-devsite/telemetry_.telemetryoptions.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,19 @@ export interface TelemetryOptions

| Property | Type | Description |
| --- | --- | --- |
| [appVersion](./telemetry_.telemetryoptions.md#telemetryoptionsappversion) | string | The version of the application. This should be a unique string that identifies the snapshot of code to be deployed, such as "1.0.2". If not specified, other default locations will be checked for an identifier. Setting a value here takes precedent over any other values. |
| [endpointUrl](./telemetry_.telemetryoptions.md#telemetryoptionsendpointurl) | string | The URL for the endpoint to which telemetry data should be sent, in the Open Telemetry format. By default, data will be sent to Firebase. |

## TelemetryOptions.appVersion

The version of the application. This should be a unique string that identifies the snapshot of code to be deployed, such as "1.0.2". If not specified, other default locations will be checked for an identifier. Setting a value here takes precedent over any other values.

<b>Signature:</b>

```typescript
appVersion?: string;
```

## TelemetryOptions.endpointUrl

The URL for the endpoint to which telemetry data should be sent, in the Open Telemetry format. By default, data will be sent to Firebase.
Expand Down
11 changes: 11 additions & 0 deletions docs-devsite/telemetry_angular.telemetryoptions.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,19 @@ export interface TelemetryOptions

| Property | Type | Description |
| --- | --- | --- |
| [appVersion](./telemetry_angular.telemetryoptions.md#telemetryoptionsappversion) | string | The version of the application. This should be a unique string that identifies the snapshot of code to be deployed, such as "1.0.2". If not specified, other default locations will be checked for an identifier. Setting a value here takes precedent over any other values. |
| [endpointUrl](./telemetry_angular.telemetryoptions.md#telemetryoptionsendpointurl) | string | The URL for the endpoint to which telemetry data should be sent, in the Open Telemetry format. By default, data will be sent to Firebase. |

## TelemetryOptions.appVersion

The version of the application. This should be a unique string that identifies the snapshot of code to be deployed, such as "1.0.2". If not specified, other default locations will be checked for an identifier. Setting a value here takes precedent over any other values.

<b>Signature:</b>

```typescript
appVersion?: string;
```

## TelemetryOptions.endpointUrl

The URL for the endpoint to which telemetry data should be sent, in the Open Telemetry format. By default, data will be sent to Firebase.
Expand Down
11 changes: 11 additions & 0 deletions docs-devsite/telemetry_react.telemetryoptions.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,19 @@ export interface TelemetryOptions

| Property | Type | Description |
| --- | --- | --- |
| [appVersion](./telemetry_react.telemetryoptions.md#telemetryoptionsappversion) | string | The version of the application. This should be a unique string that identifies the snapshot of code to be deployed, such as "1.0.2". If not specified, other default locations will be checked for an identifier. Setting a value here takes precedent over any other values. |
| [endpointUrl](./telemetry_react.telemetryoptions.md#telemetryoptionsendpointurl) | string | The URL for the endpoint to which telemetry data should be sent, in the Open Telemetry format. By default, data will be sent to Firebase. |

## TelemetryOptions.appVersion

The version of the application. This should be a unique string that identifies the snapshot of code to be deployed, such as "1.0.2". If not specified, other default locations will be checked for an identifier. Setting a value here takes precedent over any other values.

<b>Signature:</b>

```typescript
appVersion?: string;
```

## TelemetryOptions.endpointUrl

The URL for the endpoint to which telemetry data should be sent, in the Open Telemetry format. By default, data will be sent to Firebase.
Expand Down
33 changes: 29 additions & 4 deletions packages/telemetry/src/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ describe('Top level API', () => {
expect(log.body).to.equal('This is a test error');
expect(log.attributes).to.deep.equal({
'error.type': 'TestError',
'error.stack': '...stack trace...'
'error.stack': '...stack trace...',
'app.version': 'unset'
});
});

Expand All @@ -140,7 +141,8 @@ describe('Top level API', () => {
expect(log.body).to.equal('error with no stack');
expect(log.attributes).to.deep.equal({
'error.type': 'Error',
'error.stack': 'No stack trace available'
'error.stack': 'No stack trace available',
'app.version': 'unset'
});
});

Expand All @@ -151,7 +153,9 @@ describe('Top level API', () => {
const log = emittedLogs[0];
expect(log.severityNumber).to.equal(SeverityNumber.ERROR);
expect(log.body).to.equal('a string error');
expect(log.attributes).to.deep.equal({});
expect(log.attributes).to.deep.equal({
'app.version': 'unset'
});
});

it('should capture an unknown error type correctly', () => {
Expand All @@ -161,7 +165,9 @@ describe('Top level API', () => {
const log = emittedLogs[0];
expect(log.severityNumber).to.equal(SeverityNumber.ERROR);
expect(log.body).to.equal('Unknown error type: number');
expect(log.attributes).to.deep.equal({});
expect(log.attributes).to.deep.equal({
'app.version': 'unset'
});
});

it('should propagate trace context', async () => {
Expand All @@ -187,6 +193,7 @@ describe('Top level API', () => {
expect(emittedLogs[0].attributes).to.deep.equal({
'error.type': 'TestError',
'error.stack': '...stack trace...',
'app.version': 'unset',
'logging.googleapis.com/trace': `projects/${PROJECT_ID}/traces/my-trace`,
'logging.googleapis.com/spanId': `my-span`
});
Expand All @@ -211,6 +218,7 @@ describe('Top level API', () => {
expect(log.attributes).to.deep.equal({
'error.type': 'TestError',
'error.stack': '...stack trace...',
'app.version': 'unset',
strAttr: 'string attribute',
mapAttr: {
boolAttr: true,
Expand All @@ -219,6 +227,23 @@ describe('Top level API', () => {
arrAttr: [1, 2, 3]
});
});

it('should use explicit app version when provided', () => {
const telemetry = {
...fakeTelemetry,
options: {
appVersion: '1.0.0'
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This test creates a plain object that structurally mimics a TelemetryService instance. This approach is not type-safe and relies on the internal implementation detail of captureError casting the telemetry object. A more robust and type-safe approach would be to instantiate TelemetryService directly, which will make the test less brittle to future refactoring.

Suggested change
const telemetry = {
...fakeTelemetry,
options: {
appVersion: '1.0.0'
}
};
const telemetry = new TelemetryService(
fakeTelemetry.app,
fakeTelemetry.loggerProvider
);
telemetry.options = {
appVersion: '1.0.0'
};


captureError(telemetry, 'a string error');

expect(emittedLogs.length).to.equal(1);
const log = emittedLogs[0];
expect(log.attributes).to.deep.equal({
'app.version': '1.0.0'
});
});
});

describe('flush()', () => {
Expand Down
34 changes: 20 additions & 14 deletions packages/telemetry/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,13 @@ export function getTelemetry(
TELEMETRY_TYPE
);
const identifier = options?.endpointUrl || '';
return telemetryProvider.getImmediate({ identifier });
const telemetry: TelemetryService = telemetryProvider.getImmediate({
identifier
});
if (options) {
telemetry.options = options;
}
return telemetry;
}

/**
Expand All @@ -72,20 +78,27 @@ export function captureError(
attributes?: AnyValueMap
): void {
const logger = telemetry.loggerProvider.getLogger('error-logger');
const customAttributes = attributes || {};

// Add trace metadata
const activeSpanContext = trace.getActiveSpan()?.spanContext();
const traceAttributes = {} as AnyValueMap;
if (telemetry.app.options.projectId && activeSpanContext?.traceId) {
traceAttributes[
customAttributes[
'logging.googleapis.com/trace'
] = `projects/${telemetry.app.options.projectId}/traces/${activeSpanContext.traceId}`;
if (activeSpanContext?.spanId) {
traceAttributes['logging.googleapis.com/spanId'] =
customAttributes['logging.googleapis.com/spanId'] =
activeSpanContext.spanId;
}
}

const customAttributes = attributes || {};
// Add app version metadata
let appVersion = 'unset';
// TODO: implement app version fallback logic
if ((telemetry as TelemetryService).options?.appVersion) {
appVersion = (telemetry as TelemetryService).options!.appVersion!;
}
Comment on lines +98 to +100
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The current check for appVersion uses a truthiness check, which will incorrectly handle an empty string ('') as a version. If a user explicitly provides an empty string, it will be treated as falsy, and the appVersion will default to 'unset'. To correctly handle all string values, including empty ones, you should check for null or undefined instead.

Suggested change
if ((telemetry as TelemetryService).options?.appVersion) {
appVersion = (telemetry as TelemetryService).options!.appVersion!;
}
if ((telemetry as TelemetryService).options?.appVersion != null) {
appVersion = (telemetry as TelemetryService).options.appVersion;
}

Copy link
Author

Choose a reason for hiding this comment

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

An empty string should map to unset

customAttributes['app.version'] = appVersion;

if (error instanceof Error) {
logger.emit({
Expand All @@ -94,27 +107,20 @@ export function captureError(
attributes: {
'error.type': error.name || 'Error',
'error.stack': error.stack || 'No stack trace available',
...traceAttributes,
...customAttributes
}
});
} else if (typeof error === 'string') {
logger.emit({
severityNumber: SeverityNumber.ERROR,
body: error,
attributes: {
...traceAttributes,
...customAttributes
}
attributes: customAttributes
});
} else {
logger.emit({
severityNumber: SeverityNumber.ERROR,
body: `Unknown error type: ${typeof error}`,
attributes: {
...traceAttributes,
...customAttributes
}
attributes: customAttributes
});
}
}
Expand Down
7 changes: 7 additions & 0 deletions packages/telemetry/src/public-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,11 @@ export interface TelemetryOptions {
* By default, data will be sent to Firebase.
*/
endpointUrl?: string;

/**
* The version of the application. This should be a unique string that identifies the snapshot of
* code to be deployed, such as "1.0.2". If not specified, other default locations will be checked
* for an identifier. Setting a value here takes precedent over any other values.
*/
appVersion?: string;
}
12 changes: 11 additions & 1 deletion packages/telemetry/src/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,23 @@
*/

import { _FirebaseService, FirebaseApp } from '@firebase/app';
import { Telemetry } from './public-types';
import { Telemetry, TelemetryOptions } from './public-types';
import { LoggerProvider } from '@opentelemetry/sdk-logs';

export class TelemetryService implements Telemetry, _FirebaseService {
_options?: TelemetryOptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The backing field _options for the options property is currently public. To ensure proper encapsulation and prevent direct modification from outside the class, it should be made private. The public options getter and setter are the intended way to interact with this property.

Suggested change
_options?: TelemetryOptions;
private _options?: TelemetryOptions;


constructor(public app: FirebaseApp, public loggerProvider: LoggerProvider) {}

_delete(): Promise<void> {
return Promise.resolve();
}

set options(optionsToSet: TelemetryOptions) {
this._options = optionsToSet;
}

get options(): TelemetryOptions | undefined {
return this._options;
}
}
Loading