[APM] Fix query to mobile transactions main statistics

[MiriamAparicio]

When splitting the query to mobile transactions main statistics, the sessions were left in the errors query and should have been retrieved in the transactions events. This PR fix this issue

[elasticmachine]

Pinging @elastic/apm-ui (Team:APM)

[apmmachine]

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

• /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[kpatticha]

Also, I think this is not used

[MiriamAparicio]

This type is used in line 194, needed to set which type we want the array returned by merge()

[kpatticha]

I see! I don't fully understand the following if you could please elaborate a bit more

export interface MobileMainStatisticsResponse {
  mainStatistics: Array<{
    name: string | number;
    latency: number | null;
    throughput: number;
    crashRate: number;
  }>;
}

type MainStatistics = Array<{
  name: string | number;
  latency: number | null;
  throughput: number;
  sessions: number;
  crashes?: number;
}>;

cause I would expect to be

export interface MobileMainStatisticsResponse {
  mainStatistics: MainStatistics
}

why do you have to calculate the crashRate here ->

kibana/x-pack/plugins/apm/server/routes/mobile/get_mobile_main_statistics_by_field.ts

Line 204 in b09ec7a

crashRate: item.crashes ? item.crashes / item.sessions : 0,

[MiriamAparicio]

The merge of the two query responses is different from the return we want, for instance, we need sessions to calculate the crashRate, but we don't need them to be returned by getMobileMainStatisticsByField(), the main statistics we need are name, latency, throughput, and crashRate.
We calculate the crashRate here, same as we also calculate the throughput

[MiriamAparicio]

Maybe the naming is confusing, I can give it a thought and find a more clear name for the type

[MiriamAparicio]

Oh I see the confusion, sessions shouldn’t be part of errors query, does it make sense now?

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: c01244e

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
apm	3.5MB	3.5MB	-12.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
enterpriseSearch	17	19	+2
securitySolution	399	402	+3
total			+5

Total ESLint disabled count

id	before	after	diff
enterpriseSearch	18	20	+2
securitySolution	479	482	+3
total			+5

History

• 💛 Build #123956 was flaky c2a41e1
• 💚 Build #123575 succeeded b09ec7a
• 💔 Build #123545 failed 5751fb7

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[kpatticha]

getMobileMainStatisticsByField(){
   async function getMobileTransactionEventStatistics() {}
   async function getMobileErrorEventStatistics(){}
}

Nesting functions can be useful for encapsulating functionality and scoping variables but in this case, it seems it doesn't provide any value.

On the other hand, if there is a possibility of reusing ex getMobileErrorEventStatistics in other parts of the codebase, defining it outside of getMobileMainStatisticsByField could be a more practical approach.

What do you think about defining them outside of getMobileMainStatisticsByField?

[MiriamAparicio]

I thought about it, but getMobileErrorEventStatistics is getting crash errors by field, it's not generic enough to be reusable. But I will keep it in mind for when we start implementing more use cases for crashRate

[kpatticha]

but getMobileErrorEventStatistics is getting crash errors by field, it's not generic enough to be reusable.

tbh, I'm not sure what you meant with it 🤔 but I think something like the following will work

const commonPros = {
    kuery,
    apmEventClient,
    serviceName,
    environment,
    start,
    end,
    field,
  };
  const [transactioEventStatistics, errorEventStatistics] = await Promise.all([
    getMobileTransactionEventStatistics(commonPros),
    getMobileErrorEventStatistics(commonPros),
  ]);

[kpatticha]

LGTM, I only left a comment about the nesting functions

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation