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

feat(angular): Auto-detect component name in TraceDirective #6223

Closed
wants to merge 3 commits into from

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Nov 17, 2022

WIP (Still fighting tests, will come back to it later)
(Also, if we merge this, we'll need to adjust docs)

Thought I'd give this a quick try after working on #6222

This PR adds auto detection of the component name in which TraceDirective is used. This makes it unnecessary for users in the future to specifiy a name in the trace directive unless they want to:

<!-- span description: <CustomName> -->
<app-sample-component trace="CustomName"></app-sample-component>
<!-- New: span description: <app-sample-component>, previously <unknown> -->
<app-sample-component trace></app-sample-component>

The auto detection currently takes the component selector name (app-sample-component in the example above). We could theoretically also take the class name (see TODO in code). Both approaches are admittedly quite hacky but the selector one is a little bit less hacky, which is why I opted for it right now.

In case we cannot auto-obtain the selector, we still fall back to the UNKNONW_COMPONENT default.

Tested this with all Angular versions from 10 to 15 and it seems to work in my test apps.

@Lms24 Lms24 self-assigned this Nov 17, 2022
@Lms24
Copy link
Member Author

Lms24 commented Nov 17, 2022

Hmm @onurtemizkan do you by any chance have an idea how I can fix the tests here? I tried playing around with the TestEnv.setup function but didn't get anywhere, yet

@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.79 KB (+0.01% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 61.3 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.57 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 54.84 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.34 KB (0%)
@sentry/browser - Webpack (minified) 66.48 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.36 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.56 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.75 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.19 KB (+0.02% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 42.09 KB (-0.01% 🔽)
@sentry/replay - Webpack (gzipped + minified) 38.17 KB (0%)

Base automatically changed from lms-fix-tracedirective-unknown-cmp to master November 17, 2022 13:42
@github-actions
Copy link
Contributor

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@AbhiPrasad
Copy link
Member

Going to close this because stale, let's re-open once we start work on this again.

@AbhiPrasad AbhiPrasad closed this Jan 30, 2023
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.

None yet

2 participants