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

Support prescribed labels #3848

Merged
merged 48 commits into from
Feb 20, 2024
Merged

Support prescribed labels #3848

merged 48 commits into from
Feb 20, 2024

Conversation

Konstantinov-Innokentii
Copy link
Member

@Konstantinov-Innokentii Konstantinov-Innokentii commented Feb 7, 2024

What this PR does

Cleanup label typing:

  1. LabelParam -> two separate types LabekKey and LabelValue
  2. LabelData -> renamed to LabelPair.
  3. LabelKeyData -> renamed to LabelOption
    Data is not giving any info about what this type represents.
  4. Remove LabelsData and LabelsKeysData types. They are just list of types listed above and with new naming it feels obsolete.
  5. ValueData removed. LabelPair is used instead.
  6. Rework AlertGroupCustomLabel to use LabelKey type for key to make type system more consistent. Name model type AlertGroupCustomLabelDB and api type AlertGroupCustomLabelAPI to clearly distinguish them.

Split update_labels_cache into two tasks update_label_option_cache and update_label_pairs_cache.
Original task was expecting array of LabelsData (now it's LabelPair) OR one LabelKeyData ( now it's LabelOption). I believe having one function with two sp different argument types makes it more complicated for understanding.

Make OnCall backend support prescribed labels. OnCall will sync and store "prescribed" field for key and values, so Label dropdown able to disable editing for certain labels.

Which issue(s) this PR fixes

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

@Konstantinov-Innokentii Konstantinov-Innokentii added the pr:no public docs Added to a PR that does not require public documentation updates label Feb 7, 2024
@@ -16,32 +16,28 @@
ASSOCIATED_MODEL_NAME = "AssociatedLabel"


class LabelUpdateParam(typing.TypedDict):
class Key(typing.TypedDict):
Copy link
Member

Choose a reason for hiding this comment

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

I'd propose to rename it to LabelKey



class LabelParams(typing.TypedDict):
class Value(typing.TypedDict):
Copy link
Member

Choose a reason for hiding this comment

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

I'd propose to rename it to LabelValue

engine/apps/labels/tasks.py Outdated Show resolved Hide resolved
engine/apps/labels/models.py Outdated Show resolved Hide resolved
@Konstantinov-Innokentii Konstantinov-Innokentii marked this pull request as ready for review February 13, 2024 07:35
@Konstantinov-Innokentii Konstantinov-Innokentii requested a review from a team February 13, 2024 07:35
Copy link
Member

@vstpme vstpme left a comment

Choose a reason for hiding this comment

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

LGTM. Consider adding simple unit tests for the new Celery tasks

self._update_labels_cache(result)
return Response(result, status=response.status_code)
value, response = LabelsAPIClient(organization.grafana_url, organization.api_token).get_value(key_id, value_id)
# TODO: update_labels_cache expects LabelOption, but get value returns a Value. Investigate, temporary disable.
Copy link
Member

Choose a reason for hiding this comment

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

Does this TODO need to be addressed before merging the PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's for future iterations.

update_labels_cache.apply_async((labels_data,))
# Many labels
update_label_pairs_cache.apply_async((label_pairs,))
# update_labels_cache.apply_async((label_pairs,))
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this line should be deleted

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks

@Konstantinov-Innokentii Konstantinov-Innokentii merged commit acd0c44 into dev Feb 20, 2024
20 of 21 checks passed
@Konstantinov-Innokentii Konstantinov-Innokentii deleted the prescribed-labels branch February 20, 2024 06:42
@joeyorlando
Copy link
Contributor

just FYI for next time, this PR introduced linting issues into dev

@Konstantinov-Innokentii
Copy link
Member Author

@joeyorlando Oh, I thought it was introduced here. Did I bring new one?

brojd pushed a commit that referenced this pull request Sep 18, 2024
# What this PR does

**Cleanup label typing:**
1. LabelParam -> two separate types LabekKey and LabelValue 
2. LabelData -> renamed to LabelPair. 
3. LabelKeyData -> renamed to LabelOption
Data is not giving any info about what this type represents. 
4. Remove LabelsData and LabelsKeysData types. They are just list of
types listed above and with new naming it feels obsolete.
5. ValueData removed. LabelPair is used instead.
6. Rework AlertGroupCustomLabel to use LabelKey type for key to make
type system more consistent. Name model type AlertGroupCustomLabel**DB**
and api type AlertGroupCustomLabel**API** to clearly distinguish them.

**Split update_labels_cache into two tasks** update_label_option_cache
and update_label_pairs_cache.
Original task was expecting array of LabelsData (now it's LabelPair) OR
one LabelKeyData ( now it's LabelOption). I believe having one function
with two sp different argument types makes it more complicated for
understanding.


**Make OnCall backend support prescribed labels**. OnCall will sync and
store "prescribed" field for key and values, so Label dropdown able to
disable editing for certain labels.

## Which issue(s) this PR fixes

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [ ] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)

---------

Co-authored-by: Maxim Mordasov <maxim.mordasov@grafana.com>
Co-authored-by: Yulya Artyukhina <Ferril.darkdiver@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants