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

Code enhancements for future use #198

Merged
merged 5 commits into from
May 17, 2024
Merged

Code enhancements for future use #198

merged 5 commits into from
May 17, 2024

Conversation

yhabteab
Copy link
Member

This PR proposes the following improvements in preparation for #188.

  • 9502b20 decouples contactChannels from the incident package, since we're going to use this recipient channels map elsewhere outside the incident package in the future and it seems reasonable to me to move it here to the rule package.
  • d238b0b implements zapcore.ObjectMarshaler for the rule.Escalation and recipient.Key types and allows us to use zap.Inline(escalation) or zap.Object("escalation", escalation)wherever more logging context is needed without having to add all the individual fields ourselves each time.
  • ce8732e moves the ErrSuperfluousStateChange error type to the event package in order to mitigate a cyclic import error with the upcoming changes to non-state event processing support. It kind of also fit into this package.
  • 706ed38 drops incident#AddHistory() method as it was already quite annoying having to return types.Int every single time this method was used. This commit embeds the functionality for insert and fetch ID directly into the HistoryRow type and nowhere needs to be returned.
  • 7ef9aa2 drops the create param from incident#ProcessEvent() and incident#GetCurrent(), since we can easily determine whether an incident is new based on its StartedAt timestamp, and thus, we don't need to pass this info around methods. (Added @sukhwinder33445 as a co-author as the original idea was from him, but I've implemented it in a slightly different way).

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label May 16, 2024
@yhabteab yhabteab added the enhancement New feature or request label May 16, 2024
@yhabteab yhabteab requested a review from julianbrost May 16, 2024 14:46
@yhabteab yhabteab force-pushed the future-use-preparation branch from 7ef9aa2 to 0f017d9 Compare May 16, 2024 15:26
@yhabteab yhabteab force-pushed the future-use-preparation branch from 0f017d9 to a1e73e3 Compare May 17, 2024 08:07
@yhabteab
Copy link
Member Author

The output of zap.Object("escalation", escalation) looks as follows:

2024-05-17T10:05:49.375+0200    INFO    incident        Rule reached escalation {"object": "icinga-2!ping4", "incident": "#12", "rule": "default", "escalation": {"id": 1, "rule_id": 1, "name": "[S] Working Hours, [C] Yonas Habteab", "condition": "incident_severity>=ok"}}

@yhabteab yhabteab requested review from julianbrost and removed request for julianbrost May 17, 2024 08:13
internal/incident/incident.go Outdated Show resolved Hide resolved
internal/rule/rule.go Outdated Show resolved Hide resolved
internal/config/rule.go Outdated Show resolved Hide resolved
internal/rule/escalation.go Outdated Show resolved Hide resolved
internal/incident/db_types.go Outdated Show resolved Hide resolved
We're going to use this recipient channels map elsewhere outside the
`incident` package in the future and it seems reasonable to me to move
it here to the `rule` package.
@yhabteab yhabteab force-pushed the future-use-preparation branch from a1e73e3 to f482e1a Compare May 17, 2024 09:19
@yhabteab yhabteab requested a review from julianbrost May 17, 2024 09:19
yhabteab and others added 4 commits May 17, 2024 12:34
…t.Key`

This allows us to use `zap.Inline(escalation)` or
`zap.Object("escalation", escalation)`wherever more logging context is
needed without having to add all the individual fields ourselves each
time.
Otherwise, we will have a cyclic import error with the upcoming changes
to non-state event processing support. It kind of also fit into this package.
It was already quite annoying having to return `types.Int` every single
time this method was used. This commit embeds the functionality for
`insert and fetch ID` directly into the `HistoryRow` type and nowhere
needs to be returned. The just inserted and fetched ID can be accessed
with `history.ID`.
We don't need to pass this info around methods since we can easily
determine whether an incident is new based on its `StartedAt` timestamp.

Co-Authored-By: Sukhwinder Dhillon <sukhwinder.dhillon@icinga.com>
@yhabteab yhabteab force-pushed the future-use-preparation branch from f482e1a to 28704ee Compare May 17, 2024 10:35
@julianbrost julianbrost merged commit 904980c into main May 17, 2024
12 checks passed
@julianbrost julianbrost deleted the future-use-preparation branch May 17, 2024 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants