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

Prometheus: Fixes default step value for annotation query #21934

Merged
merged 1 commit into from
Feb 5, 2020

Conversation

hugohaggmark
Copy link
Contributor

@hugohaggmark hugohaggmark commented Feb 5, 2020

What this PR does / why we need it:
Introduced during a refactor here #20536.
When the step is removed in the Annotation Editor the annotation.step is an empty string ''. The original code annotation.step || '60s' handled this but in the refactor we use destructuring with a default value , step = '60s' } = annotation which only applies if step is null or undefined.

This PR adds a method to handle this case and make it easier to test. PR also adds a constant used in both Angular view code and in datasource.

Which issue(s) this PR fixes:
Closes #21914

Special notes for your reviewer:

@@ -520,20 +522,29 @@ export class PrometheusDatasource extends DataSourceApi<PromQuery, PromOptions>
};
}

createAnnotationQueryOptions = (options: any): DataQueryRequest<PromQuery> => {
const annotation = options.annotation;
const interval =
Copy link
Member

Choose a reason for hiding this comment

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

Think we should use this.adjustInterval here so that the interval adjusts to time range (so the default 60s won't end up returning too many data points for a long time range).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about that, isn't that covered in createQuery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated PR description so it's more clear what this fix does.

Copy link
Contributor

@tskarhed tskarhed left a comment

Choose a reason for hiding this comment

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

Looks good to me! 😃

@tskarhed tskarhed merged commit 26d71c9 into master Feb 5, 2020
@tskarhed tskarhed deleted the hugoh/fix-prom-annotation-step branch February 5, 2020 15:40
alexanderzobnin pushed a commit that referenced this pull request Feb 6, 2020
alexanderzobnin pushed a commit that referenced this pull request Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annotations fail validation when step field is removed
4 participants