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

[feature] Added organization filter to timeseries charts #532 #538

Merged
merged 10 commits into from
Apr 11, 2024

Conversation

pandafy
Copy link
Member

@pandafy pandafy commented Aug 25, 2023

  • Added option to define "summary_query" for metrics
  • Added functionality to handle "GROUP by tags" clause to InfluxDB client

Related to #532

@pandafy pandafy force-pushed the radius-monitoring branch 3 times, most recently from fdbb779 to 9b66c51 Compare August 29, 2023 12:33
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

I see room for improvement here:

Screenshot from 2023-08-29 16-56-03

Instead of saying "organization filter", I think we should say something which indicates what we're currently viewing, I see these possible options:

  • "All organizations" or "All available organizations" or "Organization: any"
  • org.name or "Organization: org.name`

Or any other similar solution, what do you think?

README.rst Outdated Show resolved Hide resolved
Copy link
Member

@nemesifier nemesifier 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, two observations below.

@@ -671,6 +671,8 @@ def get_query(
params = self._get_query_params(time, start_date, end_date)
params.update(additional_params)
params.update({'start_date': start_date, 'end_date': end_date})
if not params.get('organization_id') and self.config_dict.get('filter__all__'):
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can use just __all__ for simplicity and clarity to anyone familiar with python (I think we use similar constructs elsewhere)?

@@ -260,14 +260,14 @@ def get_list_query(self, query, precision='s'):
if not len(result.keys()) or result.keys()[0][1] is None:
return list(result.get_points())
# Handles query which contains "GROUP BY TAG" clause
result_points = {}
result_points = OrderedDict()
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK in recent versions of python dicts are ordered by default, isn't it?

@nemesifier nemesifier merged commit 8d8fcfd into master Apr 11, 2024
21 checks passed
@nemesifier nemesifier deleted the radius-monitoring branch April 11, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants