Skip to content

Conversation

@onurtemizkan
Copy link
Collaborator

Resolves: #3604

Added serializeAttribute option to Breadcrumbs / dom integration, to allow users to define an element attribute to be serialized in breadcrumbs instead of ids or class names.

@onurtemizkan onurtemizkan requested a review from kamilogorek as a code owner May 29, 2021 17:52
@github-actions
Copy link
Contributor

github-actions bot commented May 29, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 20.83 KB (+0.27% 🔺)
@sentry/browser - Webpack 22.05 KB (+0.25% 🔺)
@sentry/react - Webpack 22.09 KB (+0.25% 🔺)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 28.23 KB (+0.21% 🔺)

@onurtemizkan onurtemizkan force-pushed the feature/breadcrumbs-serialize-with-attr branch from 3ea684b to 9519a4d Compare May 30, 2021 13:32
@onurtemizkan onurtemizkan force-pushed the feature/breadcrumbs-serialize-with-attr branch from 9519a4d to 9deeb18 Compare May 30, 2021 17:18
Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

Solid 🥇
Let's not forget to update docs to add this new option.

@HazAT
Copy link
Member

HazAT commented May 31, 2021

@onurtemizkan before we merge, can we make sure we go through all linked comments/issues of the referenced Issues to make sure we don't miss something.
Also, lets figure out if we potentially close those as well then.

@onurtemizkan
Copy link
Collaborator Author

@HazAT, there are also requests to be able to add extra attributes to allowedAttrs, allowing to keep class names or ids while having custom attributes to show. But since we are enforcing a maximum length to generated tree strings and having both class/id and an attribute doesn't add much information IMO, that might not be very usable at the end. What do you think?

@HazAT HazAT changed the title Add serializeAttribute option to breadcrumbs / dom. feat: Add serializeAttribute option to breadcrumbs / dom. Jun 10, 2021
@HazAT HazAT enabled auto-merge (squash) June 10, 2021 08:25
@HazAT HazAT disabled auto-merge June 10, 2021 08:26
@HazAT HazAT merged commit 1e8fdd1 into master Jun 10, 2021
@HazAT HazAT deleted the feature/breadcrumbs-serialize-with-attr branch June 10, 2021 08:26
@CopyJosh
Copy link

CopyJosh commented Jun 22, 2021

Would anyone be open to the possibility of serializeAttribute being an array instead? This would allow for recognizing multiple attributes that are possibly not available on all elements.

For example,

{ 
  serializeAttribute: [
    'data-testid', // all our good devs using react-testing library
    'aria-label', // attempt to fallback on something recognizable
  ],
}
...

then something like,

const keyAttrValue = Array.isArray(keyAttr) && keyAttr.length 
  ? elem.getAttribute(keyAttr.find(attr => elem.hasAttribute(attr)))
  : null;

@kamilogorek
Copy link
Contributor

I think we could make it work. Could you please open a new issue referencing this one? Thanks

@wadefleming-nz
Copy link

Solid 🥇 Let's not forget to update docs to add this new option.

I cannot find any docs on this option. How/where do I set it? Is it done via Sentry.init ?

@kamilogorek
Copy link
Contributor

kamilogorek commented Feb 16, 2024

https://docs.sentry.io/platforms/javascript/configuration/integrations/breadcrumbs/#dom

Sentry.init({
  dsn: "https://[email protected]/0",

  integrations: [
    Sentry.breadcrumbsIntegration({
      dom: {
        serializeAttribute: ['your', 'list', 'goes', 'here']
      }
    })
  ]
})

@wadefleming-nz
Copy link

https://docs.sentry.io/platforms/javascript/configuration/integrations/breadcrumbs/#dom

Thanks very much, not sure why I couldn't find that, maybe I had a d as in serializedAttribute when I searched.

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.

[Feature] User readable dom element selectors in breadcrumbs

6 participants