-
Notifications
You must be signed in to change notification settings - Fork 224
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
consider eliding span.sync=false because it is the typical case #1881
Comments
TODOs to consider:
|
I think the agent should be sending the most true information, so if a span is async the agent should be sending us that information. Whether or not it should be displayed in the UI is a UI issue. We can decide in the UI whether or not to show that data. IMO. |
@smith Hi! Quoting @sqren from some earlier discussion: "As you can see from that slack thread this decision was not communicated super well originally so agents have implemented this differently. But the intention is still for everyone to align on only setting span.sync to true or false when the async mode is different from the default mode of the language. Søren Louv-Jansen 7 hours ago What would be a good place to capture this intent? In the APM specs? In the APM UI docs? |
Yeah so I'd disagree there but ok. "The default mode of the language" is ambiguous. If I'm using Twisted in Python or EventMachine in Ruby the "default mode" is async but the "default mode of the language" is sync. |
I agree with that. I'd change "default mode" to something like "what is typical". Speaking only for node.js, it is pretty easy to say "async is almost always the typical case". Less so for Python, as you point out. If every span is marked |
Isn't that a good example of why we should leave it up to the agent to specify what is "typical" and what is not? Always specifying |
Great discussion so far all -- just coming in with my two cents: Short Version: The Node agent's current intended behavior -- which is to start a span as Long Version: I'm aligned with @smith in that it's an agent's job to represent The Truth of what's happened to the best of its ability. My experience elsewhere tells me that if we start seeing agent data as flags for particular pieces of the UI/UX we're eventually going to end up in a situation where our implementation is tied too heavily to one UI/UX and not to another. It's better to have the agent represent what actually happened to the best of it's ability. Also -- I'd also lightly push back on the idea that Node's default behavior is async. It's true that the community of programmers around Node.js encourages asynchronous programming patterns. However by default every line of code in a Node.js program executes synchronously. It's not until a programmer taker proactive action (or the library they're using-blindly takes proactive active) to schedule something asynchronously that a span becomes async. (waves hands like a dessert hermit yes, I know, worker threads complicate this a bit -- let's save that discussion for another day -- these are not the bleeding edge features you are looking for) Also, regarding UI noise -- seeing async or sync next to every span may be noise to experienced javascript engineers working at a tech first company and programming in javascript/typescript for 95% of the day -- but I'm not sure that's our typical/ideal user. Folks are coming to these charts when there's problems. They're often not the engineers who wrote the code -- or when they are they're often developers who need to work in multiple different languages to keep their company's systems up and running. It seems like there's a value to being explicit about whether a span is considered async or not -- if only to help orient folks working with multiple-language environments. |
This is how the conversation started and the thinking was that it was valuable to display when it was "atypical" but noise to always show it. Particularly it becomes hard to spot the atypical. I still think that's the case but if we get feedback from users that they lack this information I'm good with always showing it. |
Makes sense. I'm interested to hear the other agent devs take on this so I'd appreciate if you can bring it up with them. |
Reviving this old discussion. I have created elastic/kibana#109661 to solve this issue by hiding
So the question is: are we happy with the current state (in which this issue and elastic/kibana#109661 can be closed immediately) or should we go ahead and implement elastic/kibana#109661 ? |
[Alan]
Alan, would you object to language that a "typical" Node.js traced span is async? I.e. we get away from using the term "default". tl;dr: My votes are to (a) close this issue (i.e. have the Node.js APM agent report what it knows and not attempt to only report what it thinks is "interesting") and (b) continue with elastic/kibana#109661 (though I would say "when interesting", rather than "when relevant").
|
One thing to mention:
The intention is to still show async/blocking badge unconditionally in the flyout (when clicking a span or transaction). So for people in doubt they can always get the information there. |
Closing. Alan had no objections. |
tl;dr: Setting
span.sync
totrue
orfalse
on every APM span results in unhelpful UI noise in Kibana. Let's only set it when it is "interesting". For Node.js the typical case is that spans are async (most traced work span event loop async operations). That leaves "span.sync==true" as the "interesting" case.The
span.sync
field can betrue
,false
,null
or not set per the api-server spec.Here is the Kibana code that adds a
SyncBadge
to spans in the Waterfall view and the code for theSyncBadge
element. I.e.sync: true
will showing "blocking" in the ui,sync: false
will show "async",sync: null
(or not specified) will show nothing. E.g.:From chat discussion (this came up when discussing #1878):
"Felix
I remember that we’ve talked about the usefulness of having all spans marked as sync=false in Node.js. Setting sync=false makes the UI add a async flag next to the span name. But as async is the default in Node.js, it would probably make more sense to not set the sync flag in the default async case but only set sync=true when there’s a blocking operation, which makes the UI show a blocking label. This is what the RUM agent is doing."
The text was updated successfully, but these errors were encountered: