Skip to content

rfc(feature): Support Linking Traces - #141

Merged
Lms24 merged 21 commits into
mainfrom
lms/linking-traces
Jan 10, 2025
Merged

rfc(feature): Support Linking Traces#141
Lms24 merged 21 commits into
mainfrom
lms/linking-traces

Conversation

@Lms24

@Lms24Lms24 commented Nov 8, 2024

Copy link
Copy Markdown
Member

This RFC specifies our plans to support linked traces via span links.

Since this became quite a lot of text, I recommend especially reading the Preferred Option section which specifies SDK, Relay and Storage changes. The other sections are supposed to provide context and additional information.

Rendered Document

ref: https://github.com/getsentry/projects/issues/268

@Lms24Lms24 self-assigned this Nov 8, 2024
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
@andreiborza

Copy link
Copy Markdown
Member

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

Comment threadtext/0141-linking-traces.md Outdated
@Lms24

Copy link
Copy Markdown
MemberAuthor

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

I would argue that this is up to users. If users decide to drop a span link, we just won't be able to link the respective spans together. Meaning, unless there's a strong case for consistent sampling across traces (which is still TBD) I think we can permit mutation of links just like any other span property.

Comment threadtext/0141-linking-traces.md Outdated

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for all the deep context and background. I added a few comments.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
@mjq

mjq commented Nov 26, 2024

Copy link
Copy Markdown
Member

Sorry, looks like "starting a review" with both new comments and replies got weird and duplicated a bunch of stuff in the timeline above. Sorry 😬

@Lms24
Lms24 marked this pull request as ready for review December 3, 2024 11:37
@Lms24
Lms24 marked this pull request as draft December 3, 2024 11:37
@cleptric
cleptric self-requested a review December 4, 2024 19:43
@Lms24Lms24 changed the title [WIP] rfc(feature): Support Linking Tracesrfc(feature): Support Linking TracesDec 19, 2024
@Lms24
Lms24 marked this pull request as ready for review December 19, 2024 15:29

@cleptriccleptric left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do it.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated

@smeubanksmeubank left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would go with your preferred option. Great write up, thorough and detail. Glad the session topic was address as well.

One option that is not here, is the "do nothing". i suppose, but maybe that is pedantic.

Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LTGM :shipit:

While this doesn't solve all of our long-lived trace problems, and I would love to see traces linked to sessions, this is a significant step in the right direction. This concept will solve multiple issues you thoroughly described in the RFC. It's also excellent that OTel already has this concept

Thanks for the detailed write-up. I'm looking forward to implementing this.

Comment on lines +367 to +369
It's worth noting that we will not link to the previous positively sampled trace if a negatively sampled trace is in-between (see Traces 2-4 in the diagram). Furthermore, we will not show _how many_ traces were negatively sampled in between two trace chains; only that there was at least one trace in between (see Trace 5-8).

There are some ideas how we could show multiple negatively sampled spans as well as how we could connect traces with gaps due to sampling. For now, we'll disregard this because there's no concrete use case, yet. However, given spans can have multiple links and links can have attributes, we could add this information later on, if we deem it necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most customers don't sample at 100%. They will frequently experience previously negatively sampled traces. I can imagine that some customers would like to navigate to the last positively sampled trace when most of their previous traces are negatively sampled. But it might be OK to wait for customers to ask for that feature. The current proposal would allow multiple options to achieve this without changing the APIs and the protocol.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yup. I sketched out a solution a while ago how we could add multiple links to indicate missing traces due to sampling:
image

I didn't include this in the RFC because as I wrote, it's something we can tackle if there's demand for it.

Comment threadtext/0141-linking-traces.md Outdated
Sign up for freeto 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.

10 participants

@Lms24@andreiborza@mjq@mikejihbe@mydea@philipphofmann@cleptric@chargome@s1gr1d@smeubank
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
rfc(feature): Support Linking Traces by Lms24 · Pull Request #141 · getsentry/rfcs · GitHub
Skip to content

rfc(feature): Support Linking Traces - #141

Merged
Lms24 merged 21 commits into
mainfrom
lms/linking-traces
Jan 10, 2025
Merged

rfc(feature): Support Linking Traces#141
Lms24 merged 21 commits into
mainfrom
lms/linking-traces

Conversation

@Lms24

@Lms24Lms24 commented Nov 8, 2024

Copy link
Copy Markdown
Member

This RFC specifies our plans to support linked traces via span links.

Since this became quite a lot of text, I recommend especially reading the Preferred Option section which specifies SDK, Relay and Storage changes. The other sections are supposed to provide context and additional information.

Rendered Document

ref: https://github.com/getsentry/projects/issues/268

@Lms24Lms24 self-assigned this Nov 8, 2024
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
@andreiborza

Copy link
Copy Markdown
Member

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

Comment threadtext/0141-linking-traces.md Outdated
@Lms24

Copy link
Copy Markdown
MemberAuthor

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

I would argue that this is up to users. If users decide to drop a span link, we just won't be able to link the respective spans together. Meaning, unless there's a strong case for consistent sampling across traces (which is still TBD) I think we can permit mutation of links just like any other span property.

Comment threadtext/0141-linking-traces.md Outdated

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for all the deep context and background. I added a few comments.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
@mjq

mjq commented Nov 26, 2024

Copy link
Copy Markdown
Member

Sorry, looks like "starting a review" with both new comments and replies got weird and duplicated a bunch of stuff in the timeline above. Sorry 😬

@Lms24
Lms24 marked this pull request as ready for review December 3, 2024 11:37
@Lms24
Lms24 marked this pull request as draft December 3, 2024 11:37
@cleptric
cleptric self-requested a review December 4, 2024 19:43
@Lms24Lms24 changed the title [WIP] rfc(feature): Support Linking Tracesrfc(feature): Support Linking TracesDec 19, 2024
@Lms24
Lms24 marked this pull request as ready for review December 19, 2024 15:29

@cleptriccleptric left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do it.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated

@smeubanksmeubank left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would go with your preferred option. Great write up, thorough and detail. Glad the session topic was address as well.

One option that is not here, is the "do nothing". i suppose, but maybe that is pedantic.

Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LTGM :shipit:

While this doesn't solve all of our long-lived trace problems, and I would love to see traces linked to sessions, this is a significant step in the right direction. This concept will solve multiple issues you thoroughly described in the RFC. It's also excellent that OTel already has this concept

Thanks for the detailed write-up. I'm looking forward to implementing this.

Comment on lines +367 to +369
It's worth noting that we will not link to the previous positively sampled trace if a negatively sampled trace is in-between (see Traces 2-4 in the diagram). Furthermore, we will not show _how many_ traces were negatively sampled in between two trace chains; only that there was at least one trace in between (see Trace 5-8).

There are some ideas how we could show multiple negatively sampled spans as well as how we could connect traces with gaps due to sampling. For now, we'll disregard this because there's no concrete use case, yet. However, given spans can have multiple links and links can have attributes, we could add this information later on, if we deem it necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most customers don't sample at 100%. They will frequently experience previously negatively sampled traces. I can imagine that some customers would like to navigate to the last positively sampled trace when most of their previous traces are negatively sampled. But it might be OK to wait for customers to ask for that feature. The current proposal would allow multiple options to achieve this without changing the APIs and the protocol.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yup. I sketched out a solution a while ago how we could add multiple links to indicate missing traces due to sampling:
image

I didn't include this in the RFC because as I wrote, it's something we can tackle if there's demand for it.

Comment threadtext/0141-linking-traces.md Outdated
Sign up for freeto 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.

10 participants

@Lms24@andreiborza@mjq@mikejihbe@mydea@philipphofmann@cleptric@chargome@s1gr1d@smeubank
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' rfc(feature): Support Linking Traces by Lms24 · Pull Request #141 · getsentry/rfcs · GitHub
Skip to content

rfc(feature): Support Linking Traces - #141

Merged
Lms24 merged 21 commits into
mainfrom
lms/linking-traces
Jan 10, 2025
Merged

rfc(feature): Support Linking Traces#141
Lms24 merged 21 commits into
mainfrom
lms/linking-traces

Conversation

@Lms24

@Lms24Lms24 commented Nov 8, 2024

Copy link
Copy Markdown
Member

This RFC specifies our plans to support linked traces via span links.

Since this became quite a lot of text, I recommend especially reading the Preferred Option section which specifies SDK, Relay and Storage changes. The other sections are supposed to provide context and additional information.

Rendered Document

ref: https://github.com/getsentry/projects/issues/268

@Lms24Lms24 self-assigned this Nov 8, 2024
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
@andreiborza

Copy link
Copy Markdown
Member

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

Comment threadtext/0141-linking-traces.md Outdated
@Lms24

Copy link
Copy Markdown
MemberAuthor

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

I would argue that this is up to users. If users decide to drop a span link, we just won't be able to link the respective spans together. Meaning, unless there's a strong case for consistent sampling across traces (which is still TBD) I think we can permit mutation of links just like any other span property.

Comment threadtext/0141-linking-traces.md Outdated

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for all the deep context and background. I added a few comments.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
@mjq

mjq commented Nov 26, 2024

Copy link
Copy Markdown
Member

Sorry, looks like "starting a review" with both new comments and replies got weird and duplicated a bunch of stuff in the timeline above. Sorry 😬

@Lms24
Lms24 marked this pull request as ready for review December 3, 2024 11:37
@Lms24
Lms24 marked this pull request as draft December 3, 2024 11:37
@cleptric
cleptric self-requested a review December 4, 2024 19:43
@Lms24Lms24 changed the title [WIP] rfc(feature): Support Linking Tracesrfc(feature): Support Linking TracesDec 19, 2024
@Lms24
Lms24 marked this pull request as ready for review December 19, 2024 15:29

@cleptriccleptric left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do it.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated

@smeubanksmeubank left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would go with your preferred option. Great write up, thorough and detail. Glad the session topic was address as well.

One option that is not here, is the "do nothing". i suppose, but maybe that is pedantic.

Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LTGM :shipit:

While this doesn't solve all of our long-lived trace problems, and I would love to see traces linked to sessions, this is a significant step in the right direction. This concept will solve multiple issues you thoroughly described in the RFC. It's also excellent that OTel already has this concept

Thanks for the detailed write-up. I'm looking forward to implementing this.

Comment on lines +367 to +369
It's worth noting that we will not link to the previous positively sampled trace if a negatively sampled trace is in-between (see Traces 2-4 in the diagram). Furthermore, we will not show _how many_ traces were negatively sampled in between two trace chains; only that there was at least one trace in between (see Trace 5-8).

There are some ideas how we could show multiple negatively sampled spans as well as how we could connect traces with gaps due to sampling. For now, we'll disregard this because there's no concrete use case, yet. However, given spans can have multiple links and links can have attributes, we could add this information later on, if we deem it necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most customers don't sample at 100%. They will frequently experience previously negatively sampled traces. I can imagine that some customers would like to navigate to the last positively sampled trace when most of their previous traces are negatively sampled. But it might be OK to wait for customers to ask for that feature. The current proposal would allow multiple options to achieve this without changing the APIs and the protocol.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yup. I sketched out a solution a while ago how we could add multiple links to indicate missing traces due to sampling:
image

I didn't include this in the RFC because as I wrote, it's something we can tackle if there's demand for it.

Comment threadtext/0141-linking-traces.md Outdated
Sign up for freeto 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.

10 participants

@Lms24@andreiborza@mjq@mikejihbe@mydea@philipphofmann@cleptric@chargome@s1gr1d@smeubank
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' rfc(feature): Support Linking Traces by Lms24 · Pull Request #141 · getsentry/rfcs · GitHub
Skip to content

rfc(feature): Support Linking Traces - #141

Merged
Lms24 merged 21 commits into
mainfrom
lms/linking-traces
Jan 10, 2025
Merged

rfc(feature): Support Linking Traces#141
Lms24 merged 21 commits into
mainfrom
lms/linking-traces

Conversation

@Lms24

@Lms24Lms24 commented Nov 8, 2024

Copy link
Copy Markdown
Member

This RFC specifies our plans to support linked traces via span links.

Since this became quite a lot of text, I recommend especially reading the Preferred Option section which specifies SDK, Relay and Storage changes. The other sections are supposed to provide context and additional information.

Rendered Document

ref: https://github.com/getsentry/projects/issues/268

@Lms24Lms24 self-assigned this Nov 8, 2024
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
@andreiborza

Copy link
Copy Markdown
Member

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

Comment threadtext/0141-linking-traces.md Outdated
@Lms24

Copy link
Copy Markdown
MemberAuthor

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

I would argue that this is up to users. If users decide to drop a span link, we just won't be able to link the respective spans together. Meaning, unless there's a strong case for consistent sampling across traces (which is still TBD) I think we can permit mutation of links just like any other span property.

Comment threadtext/0141-linking-traces.md Outdated

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for all the deep context and background. I added a few comments.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
@mjq

mjq commented Nov 26, 2024

Copy link
Copy Markdown
Member

Sorry, looks like "starting a review" with both new comments and replies got weird and duplicated a bunch of stuff in the timeline above. Sorry 😬

@Lms24
Lms24 marked this pull request as ready for review December 3, 2024 11:37
@Lms24
Lms24 marked this pull request as draft December 3, 2024 11:37
@cleptric
cleptric self-requested a review December 4, 2024 19:43
@Lms24Lms24 changed the title [WIP] rfc(feature): Support Linking Tracesrfc(feature): Support Linking TracesDec 19, 2024
@Lms24
Lms24 marked this pull request as ready for review December 19, 2024 15:29

@cleptriccleptric left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do it.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated

@smeubanksmeubank left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would go with your preferred option. Great write up, thorough and detail. Glad the session topic was address as well.

One option that is not here, is the "do nothing". i suppose, but maybe that is pedantic.

Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LTGM :shipit:

While this doesn't solve all of our long-lived trace problems, and I would love to see traces linked to sessions, this is a significant step in the right direction. This concept will solve multiple issues you thoroughly described in the RFC. It's also excellent that OTel already has this concept

Thanks for the detailed write-up. I'm looking forward to implementing this.

Comment on lines +367 to +369
It's worth noting that we will not link to the previous positively sampled trace if a negatively sampled trace is in-between (see Traces 2-4 in the diagram). Furthermore, we will not show _how many_ traces were negatively sampled in between two trace chains; only that there was at least one trace in between (see Trace 5-8).

There are some ideas how we could show multiple negatively sampled spans as well as how we could connect traces with gaps due to sampling. For now, we'll disregard this because there's no concrete use case, yet. However, given spans can have multiple links and links can have attributes, we could add this information later on, if we deem it necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most customers don't sample at 100%. They will frequently experience previously negatively sampled traces. I can imagine that some customers would like to navigate to the last positively sampled trace when most of their previous traces are negatively sampled. But it might be OK to wait for customers to ask for that feature. The current proposal would allow multiple options to achieve this without changing the APIs and the protocol.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yup. I sketched out a solution a while ago how we could add multiple links to indicate missing traces due to sampling:
image

I didn't include this in the RFC because as I wrote, it's something we can tackle if there's demand for it.

Comment threadtext/0141-linking-traces.md Outdated
Sign up for freeto 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.

10 participants

@Lms24@andreiborza@mjq@mikejihbe@mydea@philipphofmann@cleptric@chargome@s1gr1d@smeubank
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' rfc(feature): Support Linking Traces by Lms24 · Pull Request #141 · getsentry/rfcs · GitHub
Skip to content

rfc(feature): Support Linking Traces - #141

Merged
Lms24 merged 21 commits into
mainfrom
lms/linking-traces
Jan 10, 2025
Merged

rfc(feature): Support Linking Traces#141
Lms24 merged 21 commits into
mainfrom
lms/linking-traces

Conversation

@Lms24

@Lms24Lms24 commented Nov 8, 2024

Copy link
Copy Markdown
Member

This RFC specifies our plans to support linked traces via span links.

Since this became quite a lot of text, I recommend especially reading the Preferred Option section which specifies SDK, Relay and Storage changes. The other sections are supposed to provide context and additional information.

Rendered Document

ref: https://github.com/getsentry/projects/issues/268

@Lms24Lms24 self-assigned this Nov 8, 2024
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
@andreiborza

Copy link
Copy Markdown
Member

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

Comment threadtext/0141-linking-traces.md Outdated
@Lms24

Copy link
Copy Markdown
MemberAuthor

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

I would argue that this is up to users. If users decide to drop a span link, we just won't be able to link the respective spans together. Meaning, unless there's a strong case for consistent sampling across traces (which is still TBD) I think we can permit mutation of links just like any other span property.

Comment threadtext/0141-linking-traces.md Outdated

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for all the deep context and background. I added a few comments.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
@mjq

mjq commented Nov 26, 2024

Copy link
Copy Markdown
Member

Sorry, looks like "starting a review" with both new comments and replies got weird and duplicated a bunch of stuff in the timeline above. Sorry 😬

@Lms24
Lms24 marked this pull request as ready for review December 3, 2024 11:37
@Lms24
Lms24 marked this pull request as draft December 3, 2024 11:37
@cleptric
cleptric self-requested a review December 4, 2024 19:43
@Lms24Lms24 changed the title [WIP] rfc(feature): Support Linking Tracesrfc(feature): Support Linking TracesDec 19, 2024
@Lms24
Lms24 marked this pull request as ready for review December 19, 2024 15:29

@cleptriccleptric left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do it.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated

@smeubanksmeubank left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would go with your preferred option. Great write up, thorough and detail. Glad the session topic was address as well.

One option that is not here, is the "do nothing". i suppose, but maybe that is pedantic.

Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LTGM :shipit:

While this doesn't solve all of our long-lived trace problems, and I would love to see traces linked to sessions, this is a significant step in the right direction. This concept will solve multiple issues you thoroughly described in the RFC. It's also excellent that OTel already has this concept

Thanks for the detailed write-up. I'm looking forward to implementing this.

Comment on lines +367 to +369
It's worth noting that we will not link to the previous positively sampled trace if a negatively sampled trace is in-between (see Traces 2-4 in the diagram). Furthermore, we will not show _how many_ traces were negatively sampled in between two trace chains; only that there was at least one trace in between (see Trace 5-8).

There are some ideas how we could show multiple negatively sampled spans as well as how we could connect traces with gaps due to sampling. For now, we'll disregard this because there's no concrete use case, yet. However, given spans can have multiple links and links can have attributes, we could add this information later on, if we deem it necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most customers don't sample at 100%. They will frequently experience previously negatively sampled traces. I can imagine that some customers would like to navigate to the last positively sampled trace when most of their previous traces are negatively sampled. But it might be OK to wait for customers to ask for that feature. The current proposal would allow multiple options to achieve this without changing the APIs and the protocol.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yup. I sketched out a solution a while ago how we could add multiple links to indicate missing traces due to sampling:
image

I didn't include this in the RFC because as I wrote, it's something we can tackle if there's demand for it.

Comment threadtext/0141-linking-traces.md Outdated
Sign up for freeto 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.

10 participants

@Lms24@andreiborza@mjq@mikejihbe@mydea@philipphofmann@cleptric@chargome@s1gr1d@smeubank
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' rfc(feature): Support Linking Traces by Lms24 · Pull Request #141 · getsentry/rfcs · GitHub
Skip to content

rfc(feature): Support Linking Traces - #141

Merged
Lms24 merged 21 commits into
mainfrom
lms/linking-traces
Jan 10, 2025
Merged

rfc(feature): Support Linking Traces#141
Lms24 merged 21 commits into
mainfrom
lms/linking-traces

Conversation

@Lms24

@Lms24Lms24 commented Nov 8, 2024

Copy link
Copy Markdown
Member

This RFC specifies our plans to support linked traces via span links.

Since this became quite a lot of text, I recommend especially reading the Preferred Option section which specifies SDK, Relay and Storage changes. The other sections are supposed to provide context and additional information.

Rendered Document

ref: https://github.com/getsentry/projects/issues/268

@Lms24Lms24 self-assigned this Nov 8, 2024
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
@andreiborza

Copy link
Copy Markdown
Member

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

Comment threadtext/0141-linking-traces.md Outdated
@Lms24

Copy link
Copy Markdown
MemberAuthor

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

I would argue that this is up to users. If users decide to drop a span link, we just won't be able to link the respective spans together. Meaning, unless there's a strong case for consistent sampling across traces (which is still TBD) I think we can permit mutation of links just like any other span property.

Comment threadtext/0141-linking-traces.md Outdated

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for all the deep context and background. I added a few comments.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
@mjq

mjq commented Nov 26, 2024

Copy link
Copy Markdown
Member

Sorry, looks like "starting a review" with both new comments and replies got weird and duplicated a bunch of stuff in the timeline above. Sorry 😬

@Lms24
Lms24 marked this pull request as ready for review December 3, 2024 11:37
@Lms24
Lms24 marked this pull request as draft December 3, 2024 11:37
@cleptric
cleptric self-requested a review December 4, 2024 19:43
@Lms24Lms24 changed the title [WIP] rfc(feature): Support Linking Tracesrfc(feature): Support Linking TracesDec 19, 2024
@Lms24
Lms24 marked this pull request as ready for review December 19, 2024 15:29

@cleptriccleptric left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do it.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated

@smeubanksmeubank left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would go with your preferred option. Great write up, thorough and detail. Glad the session topic was address as well.

One option that is not here, is the "do nothing". i suppose, but maybe that is pedantic.

Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LTGM :shipit:

While this doesn't solve all of our long-lived trace problems, and I would love to see traces linked to sessions, this is a significant step in the right direction. This concept will solve multiple issues you thoroughly described in the RFC. It's also excellent that OTel already has this concept

Thanks for the detailed write-up. I'm looking forward to implementing this.

Comment on lines +367 to +369
It's worth noting that we will not link to the previous positively sampled trace if a negatively sampled trace is in-between (see Traces 2-4 in the diagram). Furthermore, we will not show _how many_ traces were negatively sampled in between two trace chains; only that there was at least one trace in between (see Trace 5-8).

There are some ideas how we could show multiple negatively sampled spans as well as how we could connect traces with gaps due to sampling. For now, we'll disregard this because there's no concrete use case, yet. However, given spans can have multiple links and links can have attributes, we could add this information later on, if we deem it necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most customers don't sample at 100%. They will frequently experience previously negatively sampled traces. I can imagine that some customers would like to navigate to the last positively sampled trace when most of their previous traces are negatively sampled. But it might be OK to wait for customers to ask for that feature. The current proposal would allow multiple options to achieve this without changing the APIs and the protocol.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yup. I sketched out a solution a while ago how we could add multiple links to indicate missing traces due to sampling:
image

I didn't include this in the RFC because as I wrote, it's something we can tackle if there's demand for it.

Comment threadtext/0141-linking-traces.md Outdated
Sign up for freeto 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.

10 participants

@Lms24@andreiborza@mjq@mikejihbe@mydea@philipphofmann@cleptric@chargome@s1gr1d@smeubank
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' rfc(feature): Support Linking Traces by Lms24 · Pull Request #141 · getsentry/rfcs · GitHub
Skip to content

rfc(feature): Support Linking Traces - #141

Merged
Lms24 merged 21 commits into
mainfrom
lms/linking-traces
Jan 10, 2025
Merged

rfc(feature): Support Linking Traces#141
Lms24 merged 21 commits into
mainfrom
lms/linking-traces

Conversation

@Lms24

@Lms24Lms24 commented Nov 8, 2024

Copy link
Copy Markdown
Member

This RFC specifies our plans to support linked traces via span links.

Since this became quite a lot of text, I recommend especially reading the Preferred Option section which specifies SDK, Relay and Storage changes. The other sections are supposed to provide context and additional information.

Rendered Document

ref: https://github.com/getsentry/projects/issues/268

@Lms24Lms24 self-assigned this Nov 8, 2024
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
@andreiborza

Copy link
Copy Markdown
Member

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

Comment threadtext/0141-linking-traces.md Outdated
@Lms24

Copy link
Copy Markdown
MemberAuthor

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

I would argue that this is up to users. If users decide to drop a span link, we just won't be able to link the respective spans together. Meaning, unless there's a strong case for consistent sampling across traces (which is still TBD) I think we can permit mutation of links just like any other span property.

Comment threadtext/0141-linking-traces.md Outdated

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for all the deep context and background. I added a few comments.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
@mjq

mjq commented Nov 26, 2024

Copy link
Copy Markdown
Member

Sorry, looks like "starting a review" with both new comments and replies got weird and duplicated a bunch of stuff in the timeline above. Sorry 😬

@Lms24
Lms24 marked this pull request as ready for review December 3, 2024 11:37
@Lms24
Lms24 marked this pull request as draft December 3, 2024 11:37
@cleptric
cleptric self-requested a review December 4, 2024 19:43
@Lms24Lms24 changed the title [WIP] rfc(feature): Support Linking Tracesrfc(feature): Support Linking TracesDec 19, 2024
@Lms24
Lms24 marked this pull request as ready for review December 19, 2024 15:29

@cleptriccleptric left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do it.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated

@smeubanksmeubank left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would go with your preferred option. Great write up, thorough and detail. Glad the session topic was address as well.

One option that is not here, is the "do nothing". i suppose, but maybe that is pedantic.

Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LTGM :shipit:

While this doesn't solve all of our long-lived trace problems, and I would love to see traces linked to sessions, this is a significant step in the right direction. This concept will solve multiple issues you thoroughly described in the RFC. It's also excellent that OTel already has this concept

Thanks for the detailed write-up. I'm looking forward to implementing this.

Comment on lines +367 to +369
It's worth noting that we will not link to the previous positively sampled trace if a negatively sampled trace is in-between (see Traces 2-4 in the diagram). Furthermore, we will not show _how many_ traces were negatively sampled in between two trace chains; only that there was at least one trace in between (see Trace 5-8).

There are some ideas how we could show multiple negatively sampled spans as well as how we could connect traces with gaps due to sampling. For now, we'll disregard this because there's no concrete use case, yet. However, given spans can have multiple links and links can have attributes, we could add this information later on, if we deem it necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most customers don't sample at 100%. They will frequently experience previously negatively sampled traces. I can imagine that some customers would like to navigate to the last positively sampled trace when most of their previous traces are negatively sampled. But it might be OK to wait for customers to ask for that feature. The current proposal would allow multiple options to achieve this without changing the APIs and the protocol.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yup. I sketched out a solution a while ago how we could add multiple links to indicate missing traces due to sampling:
image

I didn't include this in the RFC because as I wrote, it's something we can tackle if there's demand for it.

Comment threadtext/0141-linking-traces.md Outdated
Sign up for freeto 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.

10 participants

@Lms24@andreiborza@mjq@mikejihbe@mydea@philipphofmann@cleptric@chargome@s1gr1d@smeubank
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); rfc(feature): Support Linking Traces by Lms24 · Pull Request #141 · getsentry/rfcs · GitHub
Skip to content

rfc(feature): Support Linking Traces - #141

Merged
Lms24 merged 21 commits into
mainfrom
lms/linking-traces
Jan 10, 2025
Merged

rfc(feature): Support Linking Traces#141
Lms24 merged 21 commits into
mainfrom
lms/linking-traces

Conversation

@Lms24

@Lms24Lms24 commented Nov 8, 2024

Copy link
Copy Markdown
Member

This RFC specifies our plans to support linked traces via span links.

Since this became quite a lot of text, I recommend especially reading the Preferred Option section which specifies SDK, Relay and Storage changes. The other sections are supposed to provide context and additional information.

Rendered Document

ref: https://github.com/getsentry/projects/issues/268

@Lms24Lms24 self-assigned this Nov 8, 2024
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
@andreiborza

Copy link
Copy Markdown
Member

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

Comment threadtext/0141-linking-traces.md Outdated
@Lms24

Copy link
Copy Markdown
MemberAuthor

Great stuff! One thing I was wondering about is how links are affected by span filtering. What if links are actively dropped by users? Is that a concern?

I would argue that this is up to users. If users decide to drop a span link, we just won't be able to link the respective spans together. Meaning, unless there's a strong case for consistent sampling across traces (which is still TBD) I think we can permit mutation of links just like any other span property.

Comment threadtext/0141-linking-traces.md Outdated

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for all the deep context and background. I added a few comments.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md
@mjq

mjq commented Nov 26, 2024

Copy link
Copy Markdown
Member

Sorry, looks like "starting a review" with both new comments and replies got weird and duplicated a bunch of stuff in the timeline above. Sorry 😬

@Lms24
Lms24 marked this pull request as ready for review December 3, 2024 11:37
@Lms24
Lms24 marked this pull request as draft December 3, 2024 11:37
@cleptric
cleptric self-requested a review December 4, 2024 19:43
@Lms24Lms24 changed the title [WIP] rfc(feature): Support Linking Tracesrfc(feature): Support Linking TracesDec 19, 2024
@Lms24
Lms24 marked this pull request as ready for review December 19, 2024 15:29

@cleptriccleptric left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do it.

Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated
Comment threadtext/0141-linking-traces.md Outdated

@smeubanksmeubank left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would go with your preferred option. Great write up, thorough and detail. Glad the session topic was address as well.

One option that is not here, is the "do nothing". i suppose, but maybe that is pedantic.

Comment threadtext/0141-linking-traces.md
Comment threadtext/0141-linking-traces.md

@philipphofmannphilipphofmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LTGM :shipit:

While this doesn't solve all of our long-lived trace problems, and I would love to see traces linked to sessions, this is a significant step in the right direction. This concept will solve multiple issues you thoroughly described in the RFC. It's also excellent that OTel already has this concept

Thanks for the detailed write-up. I'm looking forward to implementing this.

Comment on lines +367 to +369
It's worth noting that we will not link to the previous positively sampled trace if a negatively sampled trace is in-between (see Traces 2-4 in the diagram). Furthermore, we will not show _how many_ traces were negatively sampled in between two trace chains; only that there was at least one trace in between (see Trace 5-8).

There are some ideas how we could show multiple negatively sampled spans as well as how we could connect traces with gaps due to sampling. For now, we'll disregard this because there's no concrete use case, yet. However, given spans can have multiple links and links can have attributes, we could add this information later on, if we deem it necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most customers don't sample at 100%. They will frequently experience previously negatively sampled traces. I can imagine that some customers would like to navigate to the last positively sampled trace when most of their previous traces are negatively sampled. But it might be OK to wait for customers to ask for that feature. The current proposal would allow multiple options to achieve this without changing the APIs and the protocol.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yup. I sketched out a solution a while ago how we could add multiple links to indicate missing traces due to sampling:
image

I didn't include this in the RFC because as I wrote, it's something we can tackle if there's demand for it.

Comment threadtext/0141-linking-traces.md Outdated
Sign up for freeto 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.

10 participants

@Lms24@andreiborza@mjq@mikejihbe@mydea@philipphofmann@cleptric@chargome@s1gr1d@smeubank