-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Usage collection] refactor cloud detector collector #110439
[Usage collection] refactor cloud detector collector #110439
Conversation
const response = await awsCheckedFileSystem._checkIfService(request); | ||
const response = await awsService['_checkIfService'](); | ||
expect(readFile).toBeCalledTimes(0); | ||
expect(fetchMock).toBeCalledTimes(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is asserting against the number of times fetchMock
is called not redundant considering that we assert against what the mock is called with?
In other words, are there specific situations that might cause fetch
to be called multiple times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have no way of detecting if fetch
was called multiple times by any reason without this expect
. Checking what the fetech is being called with wouldnt detect how many times the fetch was called. Having this check helps a lot when we're refactoring code in the future as it might catch unexpected changes.
imageId: 'ami-6df1e514', | ||
}, | ||
}); | ||
expect(response.toJSON()).toMatchInlineSnapshot(` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to change from asserting equality to asserting against a snapshot? Snapshots tend to get stale and I don't see anything obvious that prevents us using toEqual
.
The same question applies to the other changes from
expect(response.toJSON()).toEqual({...})
-> expect(response.toJSON()).toMatchInlineSnapshot(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are equavilant in what they are checking. using inline snapshots saves us time manually changing the values inside the object rather than just updating the snapshots and verifying they adhere to the expected changes. I dont like using variables inside the toEqual
from outside the thing that is being tested as it makes the test less verbose and might allow some bugs to pass.
zone: undefined, | ||
metadata: undefined, | ||
}); | ||
expect(response.toJSON()).toMatchInlineSnapshot(` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -37,9 +37,9 @@ export class CloudDetector { | |||
/** | |||
* Get any cloud details that we have detected. | |||
*/ | |||
getCloudDetails() { | |||
public getCloudDetails = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to look up the subtle binding differences again and cross check the change against how we're class 😅 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this!
Overall, the changes LGTM, although we could improve on where we declare and use the required cgp header.
src/plugins/kibana_usage_collection/server/collectors/cloud/detector/gcp.ts
Outdated
Show resolved
Hide resolved
These reasons make sense, but since the usage collector is running this in the background, I'm wondering if we really care about the length of time for detection? It seems we could still retry w/ backoff on 5xx errors if we wanted. I don't have particularly strong feelings on this as I don't think retries are critical for this case, but just wanted to mention it. |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Ahmad Bamieh <[email protected]>
…-link-to-kibana-app * 'master' of github.com:elastic/kibana: (120 commits) [TSVB] Support custom field format (elastic#101245) [VisEditors] Add code ownership to the functional tests (elastic#111680) [Lens] Make Lens saved object share-capable (elastic#111403) [Graph] Make Graph saved object share-capable (elastic#111404) [Stack Monitoring] Add breadcrumb support (elastic#111850) Update Jira Cloud to use OAuth2.0 (elastic#111493) Show warning message when attempting to create an APM alert in stack management (elastic#111781) Skip suite blocking ES snapshot promotion (elastic#111907) Respect `auth_provider_hint` if session is not authenticated. (elastic#111521) Added in 'Responses' field in alert telemetry & updated test (elastic#111892) [Usage collection] refactor cloud detector collector (elastic#110439) Make classnames a shared dep (elastic#111636) Fix link to e2e tests in APM testing.md (elastic#111869) [Security Solution] Add host.os.name.caseless mapping and runtime field (elastic#111455) [APM] Removes the beta label from APM tutorial (elastic#111499) (elastic#111828) [RAC] [Observability] Expand Observability alerts page functional tests (elastic#111297) Fix extra white space on the alert table whe page size is 50 or 100 (elastic#111568) [Metrics UI] Add Inventory Timeline open/close state to context and URL state (elastic#111034) [Graph] Switch to SavedObjectClient.resolve (elastic#109617) [APM] Adding lambda icon (elastic#111834) ... # Conflicts: # x-pack/plugins/reporting/public/management/__snapshots__/report_listing.test.tsx.snap
Summary
request
,fs
), and mocking those in the testsrequest
lib in favor of something likenode-fetch
which we use more widely in kibana (request looks like it's mostly used in tests atm, not many other places)404
,403
, or401
) is unncessary since replaying requests will result in the same error codes.Closes #109202
Closes #96711
Testing
Using https://github.com/lukeelmers/terraform-kibana-dev to manually test the changes are working on each cloud provider:
Tested an AWS deployment
Tested an Azure deployment
Tested a GCP deployment