[Discover] Format duration fields as durations by default#222599
[Discover] Format duration fields as durations by default#222599Bluefinger wants to merge 10 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
iblancof
left a comment
There was a problem hiding this comment.
I just tested it locally and it behaves exactly as you described!
While looking into it a bit more, I found that there's a feature allowing users to customize the formatting of fields in any of the data views. With the change we're introducing, it looks like we're overwriting that configuration, which means users won't see their custom formatting anymore.
Was that something you had already considered? I’m not sure how many people actually use that configuration, but it doesn’t feel like the best idea to overwrite something users might have customized.
Maybe we could explore an alternative that respects their config?
| // If we have a duration field, override config and provide our own formatter | ||
| if (field.name.includes('duration.us')) { | ||
| return fieldFormats | ||
| .getInstance('duration', DURATION_CONFIG) |
There was a problem hiding this comment.
| .getInstance('duration', DURATION_CONFIG) | |
| .getInstance(FIELD_FORMAT_IDS.DURATION, DURATION_CONFIG) |
There was a problem hiding this comment.
That gave me runtime build errors, hence why I went with the string declaration. discover-utils is not even a plugin, but a shared platform thing, whereas the field format is a plugin, so I assume it gets loaded after discover-utils. As such, we can only use the types, but not anything else. I could be wrong here tho
There was a problem hiding this comment.
Yeah it's because kbn-discover-utils is a regular package, which can't import from plugins except for types (technically they can, but it creates transitive dependencies that can lead to errors). You could still get type safety with just the type doing something like this though:
const duration: `${FIELD_FORMAT_IDS.DURATION}` = 'duration'|
@iblancof I'll have to see how that works, but whatever we do have, it should ideally then look for this, and then fallback to the default format, which probably will matter for ES|QL mode, which is not linked to data views. |
|
|
||
| // If we have a duration field, override config and provide our own formatter if | ||
| // one isn't available from the data view. | ||
| if (field.name.includes('duration.us')) { |
There was a problem hiding this comment.
I guess this would also match duration.user? this ain't a ECS field? And that way it's not deactivateable, right? (We will discuss it, those are initial question, thx! )
There was a problem hiding this comment.
True, though do we have a field that could be a duration.user? Else we'd have to opt for regex, but at least then I can specify the .us being the suffix and no more.
EDIT: actually, just endsWith would suffice. No regex needed
There was a problem hiding this comment.
Generally, this field formatting would be applied to any field name, right? but it's just relevant for traces? What you need to consider that in Discover this formatting would apply to any data.
this change is aiming to apply to span.duration.us or transaction.duration.us, but it also would be applied to whatever.duration.us or induration.us ... I feel like a less hacky solution, could be a contextual field value formatting? So it's just displayed just in APM context? But I think we can't do this currently, right @davismcphee ?
There was a problem hiding this comment.
At the moment, there's data views that allow for setting a field format, but this does not work for ES|QL mode (which is what I want to support as well). But also, this is a formatting that would be making these duration fields more readable, though value wise, it works fine. Is there a need to keep a duration.us kind of field unformatted/string formatted?
That said, I would prefer a better solution to this that also encompasses configuring fields for ES|QL mode, so not to rely/depend on data views.
There was a problem hiding this comment.
But also, this is a formatting that would be making these duration fields more readable, though value wise, it works fine. Is there a need to keep a duration.us kind of field unformatted/string formatted?
I do agree this is a better UX for these kind of fields in the given scenario of APM, there's just the problem, that this change currently would apply to all fields ending with duration.us, originally just aiming for span.duration.us and transaction.duration.us. We can't be sure that there are other fields available and have a different purpose
and what's more like @iblancof mentioned
While looking into it a bit more, I found that there's a feature allowing users to customize the formatting of fields in any of the data views. With the change we're introducing, it looks like we're overwriting that configuration, which means users won't see their custom formatting anymore.
So users can't undo this change
The change you aim to apply goes into the direction of contextual field formatting, and also applying field formatting for ESQL. FYI @ninoslavmiskovic
I've been thinking, what could be an alternative way to do this? Maybe we could change the meta information in the mapping of these fields?
https://www.elastic.co/docs/reference/elasticsearch/mapping-reference/mapping-field-meta
There's a default field formatters functionality based on field meta values that was implemented here:
#174973
However this wouldn't work for ES|QL ... so no good non-hacky solution for this case currently comes to my mind, maybe @davismcphee @ninoslavmiskovic have ideas?
There was a problem hiding this comment.
So users can't undo this change
That is no longer the case in this PR. For matching fields, it first checks if the user has specified a formatting for that field before then defaulting to the duration formatting that we apply. There's even a unit test in this PR covering that use-case.
I've been thinking, what could be an alternative way to do this? Maybe we could change the meta information in the mapping of these fields?
The main issue here is us moving away from data views and pushing more towards ES|QL mode, so probably this convo should involve a better mechanism to apply field formatting in a way that is agnostic to data views/ES|QL mode, but that is a much bigger task. There's nothing to stop us having that convo and ensuring we revisit whatever we do here later once the better approach lands.
There was a problem hiding this comment.
I'm also skeptical of this change. It seems relatively low risk since the naming is pretty specific, but Discover is used so broadly that it can be hard to know what users expect of their data, and we've run into issues in the past implementing seemingly innocuous changes. We even got pushback after implementing default formatting based on field metadata and that's a standardized ES convention, so I think we should be cautious. Reverting this later in favour of a more robust approach once it exists also implies a (small) breaking change, which tends not to go well for us.
I agree that the best approach longer term is to fully support field formatters for ES|QL mode, and it's been an ongoing conversation. We just don't currently have an abstraction there to hook into.
That said, field metadata default formatting is already supported in ES|QL mode today, and it's less ambiguous than non-standardized (at the ES level) naming conventions:

You can even give this a try yourself:
PUT test
{
"mappings": {
"properties": {
"my_percentage": {
"type": "double",
"meta": {
"unit": "percent"
}
}
}
}
}
POST test/_doc
{
"my_percentage": 0.36
}
Isn't there a way we can ensure the mappings for traces indices have the proper metadata properties to indicate their format? That would be more useful at both the ES and Kibana level.
I don't want to flat out say no to this change, but it at very least needs some discussion, and I really think field metadata is the correct way to approach this. Let's chat about it in the One Discover sync.
There was a problem hiding this comment.
thx @davismcphee , I should have tried out if adding meta information to the mapping would work in ES|QL 🙏 Since it does, IMO this would be the non-hacky way forward. Yes, let's chat about in in the One Discover sync
maxcold
left a comment
There was a problem hiding this comment.
cloud_security_posture changes LGTM
💚 Build Succeeded
Metrics [docs]Async chunks
History
|
|
Closing in favour of elastic/elasticsearch#129418 |

Summary
This PR introduces a quick/hacky approach to formatting duration fields. Without relying on a specific identifier, it just matches any field name with
duration.usin the text, with the assumption that this is a microsecond duration field. This then apples a duration formatter withhumanizePrecisionoutput, so that large durations can be formatted into larger units other than just milliseconds.This works for both data view mode and ES|QL mode, and will format on the overview and table tabs in the document viewer.
Closes #217886
How to test
kibana.dev.ymlfile:span.duration.us/transaction.duration.usfield, the field value should be formatted as a duration, so a duration of19,000should be formatted as19 ms.