-
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
[APM] Use observer.hostname instead of observer.name #76074
Conversation
`observer.name` is not guaranteed to be set. Using `observer.hostname` _should_ allow us to approximate the amount of APM Server instances. Additionally, some of the logic was rewritten to A) reflect updates to APM Server's implementation, and B) make it more robust by querying a static date range.
…ice environments telemetry
Pinging @elastic/apm-ui (Team:apm) |
@@ -536,61 +536,52 @@ exports[`APM telemetry helpers getApmTelemetry generates a JSON object with the | |||
}, | |||
"transaction_count": { | |||
"type": "long" | |||
} |
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.
@elastic/telemetry changes to mapping in this diff.
Added @elastic/telemetry as a reviewer so they're aware of mapping changes. |
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.
LGTM, once tests are passing and if upload script is working.
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
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.
@dgieselaar do you mind issuing a PR with the mapping changes in the telemetry repo? I've created an issue in that repo and mentioned you in it :)
services_without_environment: long, | ||
services_with_multiple_environments: long, | ||
top_enviroments: keyword, |
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.
should these 3 keys be nested in a properties
object?
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.
yes! thanks. good catch
@afharo do you want to have a final look at this? or OK to merge? |
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.
LGTM! Just added 1 NIT because it sounds strange the test is returning undefined
now. But if that's intended, all good on my end 👍
@@ -220,9 +220,9 @@ exports[`Span METRIC_SYSTEM_FREE_MEMORY 1`] = `undefined`; | |||
|
|||
exports[`Span METRIC_SYSTEM_TOTAL_MEMORY 1`] = `undefined`; | |||
|
|||
exports[`Span OBSERVER_LISTENING 1`] = `undefined`; | |||
exports[`Span OBSERVER_HOSTNAME 1`] = `undefined`; |
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.
NIT: Is it OK for this to be now undefined
?
Mind that LISTENING and NAME HOSTNAME have been swapped, but NAME HOSTNAME is now returning undefined
in the tests
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.
Yeah, that's ok. hostname
is a different field, and it's not in our test data.
💚 Build SucceededBuild metricsasync chunks size
History
To update your PR or re-run it, just comment with: |
* master: [APM] Use observer.hostname instead of observer.name (elastic#76074) Legacy logging: fix remoteAddress being duplicated in userAgent field (elastic#76751) Remove legacy deprecation adapter (elastic#76753) [Security Solution][Detections] Rule forms cleanup (elastic#76138) Adds back in custom images for reporting + tests (elastic#76810) [APM] Fix overlapping transaction names (elastic#76083) [Ingest Pipelines] Add descriptions for ingest processors A-D (elastic#75975)
# Conflicts: # x-pack/plugins/apm/server/lib/apm_telemetry/index.ts # x-pack/plugins/apm/typings/elasticsearch/aggregations.ts
…' of github.com:jloleysens/kibana into ilm/fix/pre-existing-policy-with-no-existing-repository * 'ilm/fix/pre-existing-policy-with-no-existing-repository' of github.com:jloleysens/kibana: fix empty string in selected indices (elastic#76855) [Security Solution] Refactor OverviewHost and OverviewNetwork to use Search Strategy (elastic#76409) Use Search API in Timelion (sync) (elastic#75115) [telemetry] expose getIsOptedIn function in plugin start contract (elastic#75143) [ILM] Clean up remaining js files and any typings (elastic#76803) [Logs UI] Shared `<LogStream />` component (elastic#76262) [Security Solution] Add unit test for all hosts (elastic#76752) [Security Solution] Add unit test for authentications search strategy (elastic#76665) Do not apply search source data for tsvb (elastic#75137) [Security Solution] Refactor NetworkDns to use Search Strategy (elastic#76250) [SECURITY SOLUTION] Adds 'cypress:open-as-ci' command (elastic#76125) [Logs UI] Update alert executor tests (elastic#75764) [Functional] Unskip vega tests and fix flakiness (elastic#76600) [Data] Query String Input accepts classname prop (elastic#76848) [ML] Swim lane pagination for viewing by job id (elastic#76847) [Security Solution] Refactor MatrixHistogram to use Search Strategy (elastic#76603) [APM] Use the outcome field to calculate the transaction error rate chart (elastic#75528) [APM] Use observer.hostname instead of observer.name (elastic#76074) Legacy logging: fix remoteAddress being duplicated in userAgent field (elastic#76751)
…s-for-710 * 'master' of github.com:elastic/kibana: [Search] Use async es client endpoints (elastic#76872) fix empty string in selected indices (elastic#76855) [Security Solution] Refactor OverviewHost and OverviewNetwork to use Search Strategy (elastic#76409) Use Search API in Timelion (sync) (elastic#75115) [telemetry] expose getIsOptedIn function in plugin start contract (elastic#75143) [ILM] Clean up remaining js files and any typings (elastic#76803) [Logs UI] Shared `<LogStream />` component (elastic#76262) [Security Solution] Add unit test for all hosts (elastic#76752) [Security Solution] Add unit test for authentications search strategy (elastic#76665) Do not apply search source data for tsvb (elastic#75137) [Security Solution] Refactor NetworkDns to use Search Strategy (elastic#76250) [SECURITY SOLUTION] Adds 'cypress:open-as-ci' command (elastic#76125) [Logs UI] Update alert executor tests (elastic#75764) [Functional] Unskip vega tests and fix flakiness (elastic#76600) [Data] Query String Input accepts classname prop (elastic#76848) [ML] Swim lane pagination for viewing by job id (elastic#76847) [Security Solution] Refactor MatrixHistogram to use Search Strategy (elastic#76603) [APM] Use the outcome field to calculate the transaction error rate chart (elastic#75528) [APM] Use observer.hostname instead of observer.name (elastic#76074) Legacy logging: fix remoteAddress being duplicated in userAgent field (elastic#76751) # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.tsx # x-pack/plugins/index_lifecycle_management/common/types/index.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/node_allocation.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/phases/warm_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/services/api.ts
Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync. |
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com> Co-authored-by: Joel Griffith <joel.griffith@elastic.co> Co-authored-by: John Schulz <john.schulz@elastic.co> Co-authored-by: Brandon Morelli <brandon.morelli@elastic.co>
observer.name
is not guaranteed to be set. Usingobserver.hostname
should allow us toapproximate the amount of APM Server instances.
Additionally, some of the logic was rewritten to A) reflect updates to APM Server's
implementation, and B) make it more robust by querying a static date range.
This means that the telemetry we are currently collecting for aggregated transactions is broken.
I've also added some telemetry about
service.environment
which can be useful in determining how we should deal with the environment selector.