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

RUM: Add longtask to experience field on transaction schema #4226

Closed
hmdhk opened this issue Sep 22, 2020 · 10 comments · Fixed by #4230
Closed

RUM: Add longtask to experience field on transaction schema #4226

hmdhk opened this issue Sep 22, 2020 · 10 comments · Fixed by #4230
Assignees
Milestone

Comments

@hmdhk
Copy link
Contributor

hmdhk commented Sep 22, 2020

In order to make querying longtasks easier we need to add aggregation over longtasks to transactions. The RUM agent provides this aggregation under the experience field as follows:

{
    experience: {
        longtask: {
            sum: number,
            max: number,
            count: number
        }
    }
}

Shortened form:

{
    exp: {
        lt: {
            sum: number,
            max: number,
            count: number
        }
    }
}

Note: longtask is optional and might not be present in the payload.

@axw , would you please provide a timeline for this change?

@axw
Copy link
Member

axw commented Sep 22, 2020

@jahtalab thanks for adding the rationale.

What is the duration measured in? Ideally it would be milliseconds, to align with transaction and span duration fields.

Ideally we would record these all in an aggregate_metric field, but it's not yet available in 7.x. For future-proofing, I'd like to record these in Elasticsearch with the following structure:

  • experience.longtask.sum
  • experience.longtask.max
  • experience.longtask.count

Apart from lack of "max", can you please also explain why we can't use breakdown metrics for this? AIUI, we don't capture breakdown metrics for page-load transactions. Why not? Can we do that instead? Do we need long task metrics for individual transactions, or only at an aggregated level?

Note: Currently the agent doesn't shorten the fields and therefore they are sent as the above example. From the payload size perspective the gain is not significant to shorten these fields. @axw do you have a preference on this?

Unshortened is fine.

@axw , would you please provide a timeline for this change?

We should be able to get this into 7.10.

@simitt
Copy link
Contributor

simitt commented Sep 22, 2020

@jahtalab the RUM V3 endpoint already supports a couple of user experience values, sent via the shortened exp key. Were you only refering to longtask not being shortened or would the agent send the information as experience.longtask.*? (And out of curiosity - why not also shortening these fields on the Intake level?)

@vigneshshanmugam
Copy link
Member

vigneshshanmugam commented Sep 22, 2020

@simitt Copying the feedback posted on the original PR, We would like to hear server team's thoughts before doing the shortening on the fields. I am +1 on shortening the fields from RUM level to ensure consistency even though savings are not huge as Hamid mentioned.

Not really, its not just for optimisation. We have to keep the API consistent across the spec. Having all other fields as short-hand ones and having the new ones with full flexible fields doesn't make much sense.

I am leaning towards having them as short hand fields, If you are still not convinced, let's hear it from the server team

elastic/apm-agent-rum-js#900 (comment)

@hmdhk
Copy link
Contributor Author

hmdhk commented Sep 22, 2020

@simitt , It's only for the longtask field the other ones stay the same.

The main reason for not shortening these is that the impact of doing so, i.e. payload size benefit is not significant. Specially since there's a compression API in browsers that RUM agent supports already. Although not all browsers support this new API, chromium based browser do (that's already a big part of the market) and as browsers catch up the benefit of shortening fields diminishes. I think we should keep shortening for current fields specially for spans (since they outnumber transactions by a large ratio), but for new fields on transactions we should avoid it.

@simitt
Copy link
Contributor

simitt commented Sep 22, 2020

I don't have an opinion on shortening or not - was just wondering since the v3 Intake API was built with all well defined fields being shortened - IMO up to @jahtalab and @vigneshshanmugam to decide on this.

@vigneshshanmugam
Copy link
Member

@simitt Myself and @jahtalab will discuss about this in our sync and update the thread with more details. One question though, Do you see any issues with having duration under longtask? As we have duration already mapped in v3.

@simitt
Copy link
Contributor

simitt commented Sep 22, 2020

@vigneshshanmugam duration already being defined somewhere should not be an issue

@hmdhk
Copy link
Contributor Author

hmdhk commented Sep 22, 2020

@vigneshshanmugam and I discussed it, going forward we will shorten fields that are overly long but for fields like sum, count, max we will keep them as is, I've update the description on this issue to reflect this (only longtask will be shortened to lt)

@axw , we can't use the breakdown metrics for this since we currently can't generate breakdowns for page-load transactions, the reason is that we can't determine correct breakdowns based on page-load spans. We currently only send breakdowns for network events.

@axw
Copy link
Member

axw commented Sep 23, 2020

@axw , we can't use the breakdown metrics for this since we currently can't generate breakdowns for page-load transactions, the reason is that we can't determine correct breakdowns based on page-load spans. We currently only send breakdowns for network events.

I assume you mean that we can't generate correct network breakdown for page-load transactions? Breakdown metrics are just a convention really. You can create any metrics you like, and associate them with a transaction name/type. For page-load you could just emit breakdown metrics for longtasks, and not network.

Anyway, since breakdown metrics don't currently include "max", and it could be useful to have this information at the individual transaction level anyway, let's move ahead with the proposal.

@hmdhk
Copy link
Contributor Author

hmdhk commented Sep 24, 2020

That's correct @axw , another reason we can't use the metrics is that on the dashboard we need to be able to filter by url (and other attributes on the transaction) and doing a multi-step query is too expensive. In fact we were trying to do this via spans but that had the same issue with the query.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants