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

Consistent logging #133

Merged
merged 2 commits into from
Jul 18, 2024
Merged

Consistent logging #133

merged 2 commits into from
Jul 18, 2024

Conversation

yhabteab
Copy link
Member

resolves #115

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Nov 22, 2023
@yhabteab
Copy link
Member Author

@julianbrost do you still want to return wrapped errors everywhere even though they're already logged, or is it fine the way it is? And as for the HTTP response messages, Alvar has already cleaned them up with #132, so I haven't done anything in that regard here.

@yhabteab yhabteab force-pushed the consistent-logging branch 3 times, most recently from cf9bc95 to 50ef625 Compare November 22, 2023 16:25
@julianbrost
Copy link
Collaborator

In general, I'd try to keep close to typical Go conventions regarding the returned errors. So that would be wrapping the errors even if we basically dismiss the message later on. Note that wrapping the error also isn't just about the message but also allows the proper use of functions like errors.Is().

@oxzi
Copy link
Member

oxzi commented Nov 29, 2023

Maybe we could use this PR to also get rid of contractions. Currently, there is both "can't" as well as "cannot" in the code, and other contraction forms. A (most probably incomplete) grep -ri "\b[a-z]*'[a-z]*\b" also shows "doesn't", "it's", and even more.

This might not be part of the referenced issue, but fits the PR title very well.

Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

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

Within code - excluding comments - contractions there is still one "doesn't" in internal/filter/parser_test.go on line 144.

Otherwise, next to the two small comments, the diff looks good to me.

cmd/channel/email/main.go Outdated Show resolved Hide resolved
internal/listener/listener.go Outdated Show resolved Hide resolved
@yhabteab yhabteab force-pushed the consistent-logging branch 2 times, most recently from f6e5a22 to 65de34e Compare November 30, 2023 09:44
@yhabteab yhabteab requested a review from oxzi November 30, 2023 09:44
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

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

Sorry, I have missed this typo the other time.

Otherwise, looks good to me.

internal/incident/incident.go Outdated Show resolved Hide resolved
oxzi
oxzi previously approved these changes Dec 1, 2023
@yhabteab
Copy link
Member Author

yhabteab commented Dec 1, 2023

Sorry! I had to adjust the time period entry a bit.

oxzi
oxzi previously approved these changes Dec 1, 2023
@julianbrost
Copy link
Collaborator

I'm surprised by the number of times where return errors.New("[...]") is replaced by just return err without wrapping it with some extra context information. Is that extra error message that was used there always redundant with what's already contained in err or in other ways pointless to add?

@yhabteab
Copy link
Member Author

Is that extra error message that was used there always redundant with what's already contained in err or in other ways pointless to add?

That extra error message was supposed to be used by the listener as an HTTP response message, but since we don't want to expose such specific internal error messages to the clients, there is no need in keeping those extra error messages as they're always logged with the required additional contexts.

Copy link
Collaborator

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

I don't really see the reason for rewording many of the messages. What rules are you trying to follow here or what are you trying to be consistent with? It's just a lot of places were the meaning of the messages is changed slightly and one has to check if the new message still describes what's happening. (I haven't looked at everything yet as I don't really get the point so far.)

internal/icinga2/client.go Outdated Show resolved Hide resolved
internal/icinga2/client.go Outdated Show resolved Hide resolved
@yhabteab
Copy link
Member Author

I don't really see the reason for rewording many of the messages. What rules are you trying to follow here or what are you trying to be consistent with? It's just a lot of places were the meaning of the messages is changed slightly and one has to check if the new message still describes what's happening. (I haven't looked at everything yet as I don't really get the point so far.)

It's used to be some Icinga Web notification rule state (success/fail) -> action (started|to start) -> subject, but I've reverted all the changes now, so I don't want to wast that much time in this PR anymore.

@yhabteab yhabteab force-pushed the consistent-logging branch 2 times, most recently from befc658 to aaa6879 Compare July 12, 2024 09:05
@julianbrost julianbrost requested a review from oxzi July 16, 2024 09:59
oxzi
oxzi previously approved these changes Jul 16, 2024
Copy link
Collaborator

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

#133 (comment):

but since we don't want to expose such specific internal error messages to the clients

#133 (review):

I'm surprised by the number of times where return errors.New("[...]") is replaced by just return err without wrapping it with some extra context information.

If you do replace these basic errors with the full internal errors, they must not be sent to the HTTP client anymore. Instead, the clients should just receive some generic "internal error, see server log".

@yhabteab yhabteab force-pushed the consistent-logging branch 2 times, most recently from 0436c16 to 0f27c35 Compare July 16, 2024 15:41
@yhabteab yhabteab self-assigned this Jul 17, 2024
internal/listener/listener.go Outdated Show resolved Hide resolved
internal/event/event.go Outdated Show resolved Hide resolved
Setting `10s` as a write timeout might not cause any problems when the
request is processed successfully, but when we e.g. have to retry any
database errors, which is `5m` by default, this will just in an i/o
timeout and the listener won't event write any hints about that, but will
silently abort the connections.
@julianbrost julianbrost merged commit 2b32ff4 into main Jul 18, 2024
12 checks passed
@julianbrost julianbrost deleted the consistent-logging branch July 18, 2024 10:34
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistently wrap errors
3 participants