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

[APM] AWS lambda estimated cost #143986

Merged
merged 12 commits into from
Oct 31, 2022

Conversation

cauemarcondes
Copy link
Contributor

@cauemarcondes cauemarcondes commented Oct 25, 2022

Part of #142328

Screen Shot 2022-10-24 at 3 02 31 PM

Screen Shot 2022-10-25 at 4 06 41 PM

@cauemarcondes cauemarcondes added release_note:skip Skip the PR/issue when compiling release notes v8.6.0 labels Oct 25, 2022
@cauemarcondes cauemarcondes requested review from a team as code owners October 25, 2022 19:23
@botelastic botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Oct 25, 2022
@elasticmachine
Copy link
Contributor

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

@@ -212,6 +213,7 @@ export async function getServiceMetadataDetails({
faasTriggerTypes: response.aggregations?.faasTriggerTypes.buckets.map(
(bucket) => bucket.key as string
),
hostArchitecture: host?.architecture,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible that a service could have N lambda functions with different architectures?

in that case, which architecture do we display in the popup?

Copy link
Contributor Author

@cauemarcondes cauemarcondes Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is possible. We agreed to show the most recent one so I'm going to sort the result.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please elaborate why the recent one cause it's a little bit confusing to me 🙈, usually in the popup we display summary metadata for the service.

when I see the following:
image

I expect both functions have arm architecture. (but it can be different)

I think, for the popup can be solved with simple terms agg on the host.architecture.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid if we show both the user might be confused about the estimated cost. As we use the most recent one there. @AlexanderWert WDYT? should we show a list of architecture here or just the most recent?

Copy link
Member

@AlexanderWert AlexanderWert Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caue The point about the recent one was in the case that someone changes the architecture of a specific function (even with APM services that have only a single function associated).
But if we have an APM service with multiple functions, I think we should show both here.

TBH I haven't thought about the cost estimation for a service that has multiple different functions (with different architectures and different memory sizes).

I think, in this case we would need to calculate the cost for each function individually and then sum up the result. Not only because of the architecture but also because they might have different memory sizes, which also influences the costs significantly. Not sure if we are doing this here already or how complicated that would be?

But I think for this first iteration it's also fine to just keep it simple and show an estimate based on the recent values. (The default setup is anyways a 1:1 mapping between function and APM service)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @AlexanderWert that I'll try to tackle this on another PR.

titleSize="s"
description={i18n.translate(
'xpack.apm.serverlessMetrics.summary.estimatedCost',
{ defaultMessage: 'Estimated costs avg.' }
Copy link
Contributor

@kpatticha kpatticha Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful to have a tooltip to inform the user how the avg cost is calculated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@boriskirov can you come up with a copy, please?

Copy link
Contributor

@kpatticha kpatticha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing most of the feedback. 🙏 Overall the code looks good to me 💯

I believe the tooltip and the discussion around the architecture in the popup don't block this PR so It's up to you in case you'd like to address them in a follow up PR :D

Thanks 🥇

const architecture = 'x86_64';
it('returns correct cost when usage is less than 6 b gb-sec', () => {
expect(
calcEstimatedCost({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be useful to have several invocations here, it would make it more clear from just reading the test how different factors influence the final result.

Comment on lines +184 to +190
uiSettingsClient
.get<string>(apmAWSLambdaPriceFactor)
.then(
(value): AWSLambdaPriceFactor =>
JSON.parse(value) as AWSLambdaPriceFactor
),
uiSettingsClient.get<number>(apmAWSLambdaRequestCostPerMillion),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no way to get multiple settings from the uiSettingsClient in one call? what does it do under the hood? does each call trigger an Elasticsearch request, or is it returned from a local cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a .getAll() function too. Under the hood .get(key) calls getAll but only returns the key received. And yes, the value is cached.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
observability 538 552 +14

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id before after diff
observability 37 40 +3

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.1MB 3.1MB +580.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observability 68.0KB 68.3KB +288.0B
Unknown metric groups

API count

id before after diff
observability 541 555 +14

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 57 63 +6
osquery 103 108 +5
securitySolution 439 443 +4
total +17

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 65 71 +6
osquery 104 110 +6
securitySolution 516 520 +4
total +18

History

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

@cauemarcondes cauemarcondes merged commit 80b7010 into elastic:main Oct 31, 2022
@cauemarcondes cauemarcondes deleted the apm-lambda-estimated-cost branch October 31, 2022 19:27
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 31, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 1, 2022
* main: (43 commits)
  [Synthetics] Step details page screenshot (elastic#143452)
  [Lens] Datatable expression types improvement. (elastic#144173)
  [packages/kbn-journeys] start apm after browser start and stop after browser is closed (elastic#144267)
  [Files] Make files namespace agnostic (elastic#144019)
  Implement base browser-side logging system (elastic#144107)
  Correct wrong multiplier for byte conversion (elastic#143751)
  [Monaco] Add JSON syntax support to the Monaco editor (elastic#143739)
  CCS Smoke Test for Remote Clusters and Index Management  (elastic#142423)
  [api-docs] Daily api_docs build (elastic#144294)
  chore(NA): include progress on Bazel tasks (elastic#144275)
  [RAM] Allow users to see event logs from all spaces they have access to (elastic#140449)
  [APM] Show recommended minimum size when going below 5 minutes (elastic#144170)
  [typecheck] delete temporary target_types dirs in packages (elastic#144271)
  [Security Solution][Endpoint] adds new alert loading utility and un-skip FTR test for endpoint (elastic#144133)
  [performance/journeys] revert data_stress_test_lens.ts journey step (elastic#144261)
  [TIP] Use search strategies in Threat Intelligence (elastic#143267)
  Optimize react-query dependencies (elastic#144206)
  [babel/node] invalidate cache when synth pkg map is updated (elastic#144258)
  [APM] AWS lambda estimated cost (elastic#143986)
  [Maps] layer group wizard (elastic#144129)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:APM - DEPRECATED Use Team:obs-ux-infra_services. v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants