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

Add trace-propagation-targets to unified API options #5382

Merged
merged 2 commits into from
Aug 5, 2022

Conversation

lforst
Copy link
Member

@lforst lforst commented Aug 4, 2022

Adds documentation for the discussed tracePropagationTargets option. More context: DACI (internal)

Related to changes in the JavaScript SDK: getsentry/sentry-javascript#5521

@lforst lforst requested review from Lms24 and AbhiPrasad August 4, 2022 11:45
@vercel
Copy link

vercel bot commented Aug 4, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
sentry-docs ✅ Ready (Inspect) Visit Preview Aug 5, 2022 at 8:23AM (UTC)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

LGTM so far. Just wondering if we should add a small example (similar to the one in the dev spec maybe?) to show what to specify here? The example could show a mix of regex and string and just a few URLs that would (not) match

src/platforms/common/configuration/options.mdx Outdated Show resolved Hide resolved
@lforst
Copy link
Member Author

lforst commented Aug 4, 2022

LGTM so far. Just wondering if we should add a small example (similar to the one in the dev spec maybe?) to show what to specify here? The example could show a mix of regex and string and just a few URLs that would (not) match

@Lms24 How would you go about putting an example here? I thought about it a bit but we can't really rely on JS syntax on this page because it is more or less generic to all SDKs so we'd have to put different examples for all the different SDKs here. Do we want to do that?

@Lms24
Copy link
Member

Lms24 commented Aug 4, 2022

Good point. So, I took another look at the file and it seems like there are some options (e.g. in the deprecated section) with code examples. However, I'm realizing that this isn't simple for our case because ultimately this option should make it to all SDKs. I think we can do two things:

  1. Within the tracePropagationTargets section, we use another component to show the code example in the specific language (I think PlatformSection is the one we use for that?). When SDKs add the new property, they'd have to add an example in their language
  2. We don't specify an example and just improve the description a little. We should explain that the list can have a mix of strings and regexes and that for strings a "contains" condition is enough (i.e. it doesn't have to be an exact match).

I'd vote for 2 as it seems the more straight forward way

@lforst lforst requested a review from Lms24 August 5, 2022 08:03
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing my feedback!

@lforst lforst merged commit dd5bece into master Aug 5, 2022
@lforst lforst deleted the lforst-trace-propagation-targets branch August 5, 2022 08:23
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants