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

add timestamp to inspector request stats #25667

Merged
merged 4 commits into from
Nov 26, 2018

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Nov 14, 2018

Add Request timestamp to request inspector stats

screen shot 2018-11-14 at 12 32 29 pm

The Kibana GIS app is using the inspector request stats to verify functional tests. Comparing timestamps provides a way for functional tests to ensure a new request has been made.

@nreese nreese added v7.0.0 Feature:Inspector Inspector infrastructure and implementations v6.6.0 labels Nov 14, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@@ -43,6 +43,13 @@ export class RequestResponder {
...(this.request.stats || {}),
...stats,
};
if (this.request.startTime) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We will always set startTime and it's a mandatory field on the interface, so no need for the if here.

@@ -43,6 +43,13 @@ export class RequestResponder {
...(this.request.stats || {}),
...stats,
};
if (this.request.startTime) {
const startDate = new Date(this.request.startTime);
this.request.stats['Request timestamp'] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since most of the vis parts already have been internationalized, you should use internationalization here directly:

import { i18n } from '@kbn/i18n';

const timestampKey = i18n.translate('common.ui.inspector.reqTimestampKey', {
  defaultMessage: 'Request timestamp'
});
this.request.stats[timestampKey] = {
  value: startDate.toISOString(),
  description: i18n.translate('common.ui.inspector.reqTimestampDescription', {
    defaultMessage: 'Request start timestamp'
  })
}

const startDate = new Date(this.request.startTime);
this.request.stats['Request timestamp'] = {
value: startDate.toISOString(),
description: 'Request start timestamp',
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also suggest making this description slightly more detailed. Something like:

Time when the start of the request has been logged.

Since we put this in the generic adapter I find it a bit hard what to put in here. Since we don't actually know if that's the start of the actual request e.g. in courier it's not, because it might wait a bit in the queue after the request has been logged before the request actually started. So we don't know much more than that's the time when the request has started been logging.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese
Copy link
Contributor Author

nreese commented Nov 23, 2018

@timroes I made the requested changes. Mind taking another look?

@nreese nreese merged commit 13eccd5 into elastic:master Nov 26, 2018
nreese added a commit to nreese/kibana that referenced this pull request Nov 26, 2018
* add timestamp to inspector request stats

* remove if wrapper, internationalize
nreese added a commit that referenced this pull request Nov 26, 2018
* add timestamp to inspector request stats

* remove if wrapper, internationalize
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Inspector Inspector infrastructure and implementations v6.6.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants