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

Metric labels do not include hostname when two queries are used #225

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

lpetrora
Copy link
Member

This change adds a custom label input box and allows the user to customize the labels. It corresponding filter is created, these variables are available:

  • $filter_site: Site name
  • $filter_host_name: Host name
  • $filter_host_in_group: Group containing the host
  • $filter_service: Service name
  • $filter_service_in_group: Group containing the service

$label is always available and contains the original label sent by Checmk.

CMK-15138

@lpetrora lpetrora force-pushed the lp-master-metric_labels-15138 branch 2 times, most recently from fbf2019 to 82362a0 Compare November 20, 2023 13:19
Copy link
Contributor

@BenediktSeidl BenediktSeidl left a comment

Choose a reason for hiding this comment

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

changing the label triggers a api call to checkmk, although it is not really needed as we already have all the data we need. can you give it a try to see if you can accomplish the updating of the lables without triggering a new api request to checkmk?
i fear we would have to change quite a lot of the current architecture, but maybe it's doable?

src/backend/rest.ts Outdated Show resolved Hide resolved
src/ui/QueryEditor.tsx Outdated Show resolved Hide resolved
src/ui/QueryEditor.tsx Outdated Show resolved Hide resolved
src/ui/components.tsx Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
@lpetrora lpetrora force-pushed the lp-master-metric_labels-15138 branch 2 times, most recently from 0fa54aa to 7d9ebc5 Compare November 27, 2023 09:38
@lpetrora
Copy link
Member Author

I was looking through the code and couldn't find an easy way to skip the API call when only the custom label field is updated.

@lpetrora lpetrora force-pushed the lp-master-metric_labels-15138 branch 6 times, most recently from 59bc84f to c3b2973 Compare November 27, 2023 15:12
Copy link
Contributor

@BenediktSeidl BenediktSeidl left a comment

Choose a reason for hiding this comment

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

regarding the "change label without backend request":

this is where the request is triggered:

// TODO: not sure if this is a dirty hack or a great solution:
// https://beta.reactjs.org/apis/react/useState#storing-information-from-previous-renders
const [prevCount, setPrevCount] = React.useState(JSON.stringify(requestSpec));
if (prevCount !== JSON.stringify(requestSpec)) {
setPrevCount(JSON.stringify(requestSpec));
onChange({ ...query, requestSpec: requestSpec });
onRunQuery();
}

this is the new url to the docs, the old one no longer works:
https://react.dev/reference/react/useState#storing-information-from-previous-renders

i thought: we could check what parts of the request spec changed, and if only the label changed we call onChange only and if we detect that parts of the filter changed, we call onRunQuery.

maybe we should also think about a better idea than this JSON.stringify thing, but this is a quest for another pr.

src/types.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/ui/QueryEditor.tsx Outdated Show resolved Hide resolved
src/ui/QueryEditor.tsx Outdated Show resolved Hide resolved
@lpetrora lpetrora force-pushed the lp-master-metric_labels-15138 branch 2 times, most recently from d64958f to bff761e Compare November 29, 2023 13:01
@BenediktSeidl
Copy link
Contributor

regarding the "change label without backend request":

as you already mentioned my last idea does not work. i looked at the code again, and maybe my next idea could work:
we could remember the last query, and the last query result in this function:

https://github.com/Checkmk/grafana-checkmk-datasource/blob/main/src/DataSource.ts#L30-L35

and if they current query is the same as the last query (minus the label change of course) we could use the old result and just interpolate the changed label.

but this feels a bit hacky. probably it does not work at all, because the time range is relative, so we will get a changed timerange with the label change, so the queries differ...

maybe the whole idea of saving one http request is a premature optimization? we can skip it for now, see how everything works out and come back to this idea if we see that its really necessary.

sorry about the confusion.

@lpetrora lpetrora force-pushed the lp-master-metric_labels-15138 branch 2 times, most recently from a3ff013 to d074ee9 Compare November 30, 2023 14:04
This change adds a custom label input box and allows the user to
customize the labels. It corresponding filter is created, these
variables are available:
* $filter_site: Site name
* $filter_host_name: Host name
* $filter_host_in_group: Group containing the host
* $filter_service: Service name
* $filter_service_in_group: Group containing the service

$label is always available and contains the original label sent by
Checmk.

CMK-15138
@lpetrora lpetrora force-pushed the lp-master-metric_labels-15138 branch from d074ee9 to f3ec1e3 Compare November 30, 2023 14:12
@lpetrora lpetrora merged commit 5b35a6d into main Dec 1, 2023
5 checks passed
@lpetrora lpetrora deleted the lp-master-metric_labels-15138 branch December 1, 2023 07:42
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants