Skip to content

Conversation

@kinyoklion
Copy link
Member

Adds the ability to specify a logger and to also use the base SDK logger when possible. The base SDK of a 3.x version does not expose the logger, but 4.x will.

There is a minimal replication of the warning level of the logging interface to allow compatibility with both the 3.x SDK and the 4.x SDK.

Prefixing is done at message time instead of as part of the logger. Potentially it could be ideal to do it in the logger, but this approach makes it clear that the interpolation supported by most browser loggers will not come into play. (Where if the logger does this prefixing it is either complex or you lose the sprintf style formatting support.)

Some earlier log messages has been added, but a logger instance was not yet available.

@kinyoklion kinyoklion requested a review from a team as a code owner January 16, 2025 16:56
@github-actions
Copy link
Contributor

@launchdarkly/js-sdk-common size report
This is the brotli compressed size of the ESM build.
Size: 19014 bytes
Size limit: 21000

@github-actions
Copy link
Contributor

@launchdarkly/js-client-sdk-common size report
This is the brotli compressed size of the ESM build.
Size: 15354 bytes
Size limit: 20000

@github-actions
Copy link
Contributor

@launchdarkly/js-client-sdk size report
This is the brotli compressed size of the ESM build.
Size: 19826 bytes
Size limit: 21000


private _setLogger() {
this._logger =
this._options.logger ?? ((this._client as any)?.logger as MinLogger) ?? fallbackLogger;
Copy link
Member Author

Choose a reason for hiding this comment

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

So, basically in the 4.x SDK there is a logger property, but in 3.x there is not.

Additionally error monitoring should always be initialized as early as possible, which is before the SDK (there are also some inter-dependencies that require this order). As a result we need some intermediate logging.

@kinyoklion kinyoklion marked this pull request as draft January 16, 2025 17:23
this._pendingEvents.push({ type, data: event });
if (this._pendingEvents.length > this._maxPendingEvents) {
// TODO: Log when pending events must be dropped. (SDK-915)
if (!this._eventsDropped) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Generally whenever a max is hit it will be hit many times. This will just log once to avoid creating log spam. This situation will generally represent misconfiguration where the client is not being registered.

Or something terribly wrong in the users application.

return breadcrumb === undefined ? undefined : filter(breadcrumb);
}

function applyBreadcrumbFilters(
Copy link
Member Author

Choose a reason for hiding this comment

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

This moved to be a private member of the BrowserTelemetryImpl for access to a logger.

breadcrumb,
);
} catch (e) {
if (!this._breadcrumbFilterError) {
Copy link
Member Author

Choose a reason for hiding this comment

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

If the filter was wrong once, then it will likely remain wrong, so we don't want to log spam in that situation.

@kinyoklion kinyoklion marked this pull request as ready for review January 16, 2025 20:51
@kinyoklion kinyoklion merged commit 2ef1486 into main Jan 16, 2025
22 checks passed
@kinyoklion kinyoklion deleted the rlamb/emsr-6/browser-telemetry-logging branch January 16, 2025 23:20
@github-actions github-actions bot mentioned this pull request Jan 22, 2025
kinyoklion pushed a commit that referenced this pull request Jan 22, 2025
🤖 I have created a release *beep* *boop*
---


<details><summary>browser-telemetry: 0.1.0</summary>

##
[0.1.0](browser-telemetry-v0.0.9...browser-telemetry-v0.1.0)
(2025-01-22)


### ⚠ BREAKING CHANGES

* Updated AI config interface.
([#697](#697))

### Features

* Add basic logging support for browser-telemetry.
([#736](#736))
([2ef1486](2ef1486))
* Add browser telemetry options.
([#675](#675))
([c8352b2](c8352b2))
* Add browser-telemetry API types.
([#669](#669))
([89967ee](89967ee))
* Add DOM collectors.
([#672](#672))
([4473a06](4473a06))
* Add http collectors.
([#673](#673))
([6e60ddc](6e60ddc))
* Add singleton support for browser-telemetry.
([#739](#739))
([68a3b87](68a3b87))
* Add stack trace parsing.
([#676](#676))
([ca1dd49](ca1dd49))
* Add support for breadcrumb filtering.
([#733](#733))
([5c327a1](5c327a1))
* Add support for the session init event.
([320c07d](320c07d))
* Add the ability to filter errors.
([#743](#743))
([5cffb2b](5cffb2b))
* Export browser-telemetry initialization method.
([d1b364e](d1b364e))
* Implement browser telemetry client.
([#691](#691))
([db74a99](db74a99))
* Make browser-telemetry specific inspector type.
([#741](#741))
([14ecdb3](14ecdb3))
* Random uuid for telemetry package.
([#689](#689))
([4cf34f9](4cf34f9))
* Rename initializeTelemetryInstance to initTelemetryInstance for
consistency with initTelemetry.
([257734f](257734f))
* Source maps with inline sources for browser-telemetry.
([#735](#735))
([1656a85](1656a85))
* Updated AI config interface.
([#697](#697))
([cd72ea8](cd72ea8))
* Vendor TraceKit
([d1b364e](d1b364e))


### Bug Fixes

* Clear pending events buffer when registered.
([#727](#727))
([b6ad7df](b6ad7df))
* Export BrowserTelemetry, BrowserTelemetryInspector, and
ImplementsCrumb.
([257734f](257734f))
* Fix breadcrumb filter option parsing.
([#742](#742))
([833f4ce](833f4ce))
* Remove BrowserTelemetry until more types are available.
([#671](#671))
([796b8a3](796b8a3))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants