-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Server timings #258915
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
Merged
Merged
Server timings #258915
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
f8d0f5c
add total time
drewdaemon 0eae3ef
add support for custom events
drewdaemon 654a977
automatically record ES requests
drewdaemon a51791a
clarify that the timings are publically available
drewdaemon aa4d89f
Merge branch 'main' into server-timings
elasticmachine ea70e81
remove flag
drewdaemon 7aa5c80
gate to dev mode
drewdaemon 04365e4
remove more flags
drewdaemon 0ee1142
remove more flags
drewdaemon 19e9f61
rename
drewdaemon 1aa8fd5
Merge branch 'main' into server-timings
elasticmachine 38e5b89
Changes from node scripts/lint.js --fix
kibanamachine f5022e2
Merge branch 'server-timings' of github.com:drewdaemon/kibana into se…
drewdaemon ed6515e
Merge branch 'main' of github.com:elastic/kibana into server-timings
drewdaemon b73943a
rename API
drewdaemon bf13364
Revert CPS changes
drewdaemon 385d737
remove comments
drewdaemon 67fd817
add flag to control elasticsearch timings
drewdaemon 7680c58
escape backslashes
drewdaemon 5e3f143
rename app-total
drewdaemon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
10 changes: 10 additions & 0 deletions
10
src/core/packages/elasticsearch/client-server-internal/src/timing/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| export { getTimingRequestHandler } from './timing_request_handler'; |
104 changes: 104 additions & 0 deletions
104
...e/packages/elasticsearch/client-server-internal/src/timing/timing_request_handler.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| import type { TransportRequestParams, TransportRequestOptions } from '@elastic/elasticsearch'; | ||
| import type { KibanaRequest } from '@kbn/core-http-server'; | ||
| import { httpServerMock } from '@kbn/core-http-server-mocks'; | ||
| import { loggerMock } from '@kbn/logging-mocks'; | ||
| import { getTimingRequestHandler } from './timing_request_handler'; | ||
|
|
||
| describe('getTimingRequestHandler', () => { | ||
| const mockLogger = loggerMock.create(); | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('sets timing context with startTime', () => { | ||
| const handler = getTimingRequestHandler(); | ||
| const params: TransportRequestParams = { | ||
| method: 'GET', | ||
| path: '/_search', | ||
| }; | ||
| const options: TransportRequestOptions = {}; | ||
|
|
||
| handler({ scoped: true }, params, options, mockLogger); | ||
|
|
||
| expect(options.context).toBeDefined(); | ||
| expect((options.context as any).timingContext).toBeDefined(); | ||
| expect((options.context as any).timingContext.startTime).toBeGreaterThan(0); | ||
| expect((options.context as any).timingContext.kibanaRequest).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('includes kibanaRequest in timing context when provided', () => { | ||
| const mockRequest = httpServerMock.createKibanaRequest() as KibanaRequest; | ||
| const handler = getTimingRequestHandler(mockRequest); | ||
| const params: TransportRequestParams = { | ||
| method: 'GET', | ||
| path: '/_search', | ||
| }; | ||
| const options: TransportRequestOptions = {}; | ||
|
|
||
| handler({ scoped: true }, params, options, mockLogger); | ||
|
|
||
| expect(options.context).toBeDefined(); | ||
| expect((options.context as any).timingContext).toBeDefined(); | ||
| expect((options.context as any).timingContext.kibanaRequest).toBe(mockRequest); | ||
| expect((options.context as any).timingContext.startTime).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('creates context object if not present', () => { | ||
| const handler = getTimingRequestHandler(); | ||
| const params: TransportRequestParams = { | ||
| method: 'GET', | ||
| path: '/_search', | ||
| }; | ||
| const options: TransportRequestOptions = {}; | ||
|
|
||
| expect(options.context).toBeUndefined(); | ||
| handler({ scoped: true }, params, options, mockLogger); | ||
|
|
||
| expect(options.context).toBeDefined(); | ||
| expect((options.context as any).timingContext).toBeDefined(); | ||
| }); | ||
|
|
||
| it('preserves existing context properties', () => { | ||
| const handler = getTimingRequestHandler(); | ||
| const params: TransportRequestParams = { | ||
| method: 'GET', | ||
| path: '/_search', | ||
| }; | ||
| const existingContext = { someOtherContext: { foo: 'bar' } }; | ||
| const options: TransportRequestOptions = { | ||
| context: existingContext, | ||
| }; | ||
|
|
||
| handler({ scoped: true }, params, options, mockLogger); | ||
|
|
||
| expect((options.context as any).someOtherContext).toEqual({ foo: 'bar' }); | ||
| expect((options.context as any).timingContext).toBeDefined(); | ||
| }); | ||
|
|
||
| it('records timing at invocation time', () => { | ||
| const handler = getTimingRequestHandler(); | ||
| const params: TransportRequestParams = { | ||
| method: 'GET', | ||
| path: '/_search', | ||
| }; | ||
| const options: TransportRequestOptions = {}; | ||
|
|
||
| const beforeTime = performance.now(); | ||
| handler({ scoped: true }, params, options, mockLogger); | ||
| const afterTime = performance.now(); | ||
|
|
||
| const startTime = (options.context as any).timingContext.startTime; | ||
| expect(startTime).toBeGreaterThanOrEqual(beforeTime); | ||
| expect(startTime).toBeLessThanOrEqual(afterTime); | ||
| }); | ||
| }); |
33 changes: 33 additions & 0 deletions
33
src/core/packages/elasticsearch/client-server-internal/src/timing/timing_request_handler.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| import { performance } from 'perf_hooks'; | ||
| import type { KibanaRequest } from '@kbn/core-http-server'; | ||
| import type { OnRequestHandler } from '../create_transport'; | ||
|
|
||
| /** | ||
| * Returns an {@link OnRequestHandler} that instruments ES request timing | ||
| * and stores timing context for response-phase measurement. | ||
| * | ||
| * @param kibanaRequest - Optional KibanaRequest to attach Server-Timing measurements to | ||
| * @returns Handler that sets timing context in options.context | ||
| * | ||
| * @internal | ||
| */ | ||
| export function getTimingRequestHandler(kibanaRequest?: KibanaRequest): OnRequestHandler { | ||
| return (_ctx, _params, options, _receivedLogger) => { | ||
| if (!options.context) { | ||
| options.context = {}; | ||
| } | ||
| (options.context as any).timingContext = { | ||
| startTime: performance.now(), | ||
| kibanaRequest, | ||
| }; | ||
| }; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 wonder if this should be controlled by another config flag: I really see the potential of the HTTP timings for easy troubleshooting in production via HAR information.
However, leaking the underlying ES requests in prod is too much. But this is def interesting for devs.
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 agree that we would have to revisit if we ever let this go to prod. But I would probably favor an allow-list instead of an opt-in. Anyway, I see it as a later discussion. LMK what you think
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.
Synced with @afharo — we agreed to go with the 2-config-flag strategy to provide flexibility in the future
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.
Added here: 67fd817