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.

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

Jump to bottom

feat: Add DOM collectors. #672

Merged
merged 6 commits into from
Nov 8, 2024
Merged

feat: Add DOM collectors. #672

merged 6 commits into from
Nov 8, 2024

Conversation

kinyoklion
Copy link
Member

No description provided.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2024

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2024

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2024

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

@kinyoklion kinyoklion force-pushed the rlamb/add-dom-collectors branch from 163548a to 1530c61 Compare November 7, 2024 22:18
@kinyoklion kinyoklion marked this pull request as ready for review November 7, 2024 22:25
@kinyoklion kinyoklion requested a review from a team as a code owner November 7, 2024 22:25
@kinyoklion kinyoklion mentioned this pull request Nov 7, 2024
Merged
tanderson-ld
tanderson-ld reviewed Nov 8, 2024
View reviewed changes
packages/telemetry/browser-telemetry/src/collectors/dom/KeypressCollector.ts Show resolved Hide resolved
packages/telemetry/browser-telemetry/src/collectors/dom/KeypressCollector.ts Outdated Show resolved Hide resolved
packages/telemetry/browser-telemetry/src/collectors/dom/KeypressCollector.ts Show resolved Hide resolved
packages/telemetry/browser-telemetry/src/collectors/dom/KeypressCollector.ts
type: 'input',
level: 'info',
timestamp: Date.now(),
message: toSelector(target),
Copy link
Contributor

@tanderson-ld tanderson-ld Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is the target the focused element? In the cases where the target is undefined, is a breadcrumb without the message not useful? Perhaps it being undefined is rare.

Copy link
Member Author

Choose a reason for hiding this comment

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

All the checks are just for safety as crazy things happen in a browser. If the target is undefined we cannot really do much about that. It would probably be a bug in the browser.

packages/telemetry/browser-telemetry/src/collectors/dom/KeypressCollector.ts Outdated Show resolved Hide resolved
packages/telemetry/browser-telemetry/src/collectors/dom/KeypressCollector.ts Outdated Show resolved Hide resolved
packages/telemetry/browser-telemetry/src/collectors/dom/toSelector.ts
} = { maxDepth: 10 },
): string {
// For production we may want to consider if we additionally limit the maximum selector length.
// Limiting the components should generate reasonable selectors in most cases.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could limiting length lead to a situation where someone changes an element name and that results in one less level of element depth? Is it possible customers will attach critical infrastructure to the telemetry? I'm not experienced enough with it.

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, these are for breadcrumbs. Breadcrumbs are associated data to help you understand things involved in the path getting to some event.

Generally speaking these aren't really for programmatic use, but for visual inspection and search.

That said someone always can use them that way.

Because they are primarily for human consumption I have focused on readability.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I am going to defer a length decision for now. Basically you would need some pretty wild class names and things to make is a concrete problem.

packages/telemetry/browser-telemetry/src/collectors/dom/toSelector.ts
// https://developer.mozilla.org/en-US/docs/Web/CSS/Child_combinator
const CHILD_COMBINATOR = '>';
// Spacing around the combinator is optional, but it increases readability.
const CHILD_SEPARATOR = ` ${CHILD_COMBINATOR} `;
Copy link
Contributor

@tanderson-ld tanderson-ld Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is removing the spaces worth it for additional length? This is sorta a followup to the code comment regarding length limiting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Response in other length related comment.

@kinyoklion kinyoklion merged commit 4473a06 into main Nov 8, 2024
22 checks passed
@kinyoklion kinyoklion deleted the rlamb/add-dom-collectors branch November 8, 2024 17:35
kinyoklion added a commit that referenced this pull request Nov 8, 2024
@kinyoklion
feat: Add http collectors. (#673)
6e60ddc 
Best reviewed after: #672
@github-actions github-actions bot mentioned this pull request Jan 22, 2025
Merged
kinyoklion pushed a commit that referenced this pull request Jan 22, 2025
@github-actions
chore: release main (#748)
18e1aae 
🤖 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
Reviewers

@tanderson-ld tanderson-ld tanderson-ld approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kinyoklion @tanderson-ld