[APM] Service metrics - after merge improvements

[kpatticha]

I received some feedback after merging the PR and some nits before. Gathering all of them in this ticket to address them

changes:

1. Rename helper functions according to the outcome of the discussion we'll have below - [APM] Renaming helper functions about transaction aggegations #143299
2. use _doc_count instead of value count sub aggs - [APM] Use _doc_count from service metrics #141210
   i. unblocked by Add _doc_count to Service metrics apm-server#9143

[elasticmachine]

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

[kpatticha]

Regarding renaming the functions. it turns out that the naming can be really confusing. It would be great if we could standardize it.

Today we could have the following sources in services inventory:

1. Aggregated transaction (transaction metrics)
2. Aggregated transaction for service metrics (service metrics)
3. Transactions (event based)

Transaction metrics	Service metrics
getSearchAggregatedTransactions	getSearchAggregatedServiceMetrics
getHasAggregatedTransactions	getHasAggregatedServicesMetrics
getServiceTransactionStats	getServiceAggregatedTransactionStats
getServiceDetailedStatsPeriods	getServiceAggregatedDetailedStatsPeriods

Proposal

Transaction metrics	Service metrics
getSearchTransactionMetrics	getSearchServiceMetrics
getHasTransactionMetrics	getHasServicesMetrics
getServiceTransactionStats	getServiceMetricsTransactionStats
getServiceTransactionDetailedStatsPeriods	getServiceMetricsTransactionDetailedStatsPeriods

getServiceTransactionStats and getServiceTransactionDetailedStatsPeriods rename unchanged.

@elastic/apm-ui let me know what you think

[sorenlouv]

I'm not totally sold on getServiceTransactionStats and getServiceMetricsTransactionStats. I think it's important to signal, that this returns service stats. That's not easily decipherable from the current names. My suggestion:

getServiceTransactionStats -> getServiceStatsFromTransactions
getServiceMetricsTransactionStats -> getServiceStatsFromServiceMetrics

[kpatticha]

Thanks @sqren. Make sense

[kpatticha]

@elastic/apm-ui team if anyone has another proposal please me let me know.
Otherwise,I will proceed with the following changes:

Transaction metrics	Service metrics
getSearchAggregatedTransactions	getSearchAggregatedServiceMetrics
getHasAggregatedTransactions	getHasAggregatedServicesMetrics
getServiceStatsFromTransactions	getServiceStatsFromServiceMetrics
getServiceDetailedStatsPeriodsFromTransactions	getServiceDetailedStatsPeriodsFromServiceMetrics

[sorenlouv]

WDYT about dropping "aggregated"

getSearchAggregatedTransactions -> getSearchTransactionsEvents
getHasAggregatedTransactions -> getHasTransactionsEvents
getSearchAggregatedServiceMetrics -> getSearchServiceMetrics
getHasAggregatedServicesMetrics -> getHasServicesMetrics

[gbamparop]

getSearchAggregatedTransactions -> getSearchTransactionsEvents

@sqren Transaction event seems to refer to a transaction document instead of a transaction metric document.

[kpatticha]

getSearchTransactionsEvents is the helper which determines if we'll use aggregated transactions or not.

[kpatticha]

but now I'm thinking that getSearchTransactionsEvents could interpreted as single transaction document not the metric. 🤔

is it also confusing for other?