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

Reduce logging repeated code in event server #544

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

matheuscscp
Copy link
Member

@matheuscscp matheuscscp commented Jun 7, 2023

As discussed here, it would be beneficial to have a logger on the request context of the Event Server containing the event's involved object's main metadata, i.e. kind, name and namespace, so that multiple places in the code logging this information don't need to create such verbose three-liner logger every time.

I'm also seizing the opportunity to remove a lot of repeated code for logging events about Provider objects in various places of the Event Handler code.

@matheuscscp matheuscscp marked this pull request as draft June 7, 2023 14:29
@matheuscscp
Copy link
Member Author

Oops, I blindly replaced all the logs with kind+name+namespace 😅 Let me fix this.

@matheuscscp matheuscscp force-pushed the logger-midware branch 2 times, most recently from 914b3b2 to a3a7b13 Compare June 7, 2023 14:47
@matheuscscp matheuscscp changed the title Inject enhanced logger into request context in event server Reduce logging repeated code in event server Jun 7, 2023
@matheuscscp matheuscscp marked this pull request as ready for review June 7, 2023 14:51
internal/server/event_server.go Outdated Show resolved Hide resolved
internal/server/event_server.go Outdated Show resolved Hide resolved
@matheuscscp matheuscscp force-pushed the logger-midware branch 6 times, most recently from 7f27249 to 811a813 Compare June 12, 2023 11:31
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Left a few minor comments, but overall it looks good to me.

Pasting a sample of the new log for reference and further discussion if needed.

  {
    "level": "info",
    "ts": "2023-06-12T12:59:48.211Z",
    "logger": "event-server",
    "msg": "Dispatching event: stored artifact for commit 'Merge remote-tracking branch 'origin/master''",
    "eventInvolvedObject": {
      "kind": "GitRepository",
      "namespace": "default",
      "name": "test-1",
      "uid": "46186684-1dbc-4471-b4af-35ace378737b",
      "apiVersion": "source.toolkit.fluxcd.io/v1",
      "resourceVersion": "403472"
    }
  }

A failure log with the alert info:

  {
    "level": "error",
    "ts": "2023-06-12T13:11:23.515Z",
    "logger": "event-server",
    "msg": "failed to read secret",
    "eventInvolvedObject": {
      "kind": "GitRepository",
      "namespace": "default",
      "name": "test-1",
      "uid": "46186684-1dbc-4471-b4af-35ace378737b",
      "apiVersion": "source.toolkit.fluxcd.io/v1",
      "resourceVersion": "405229"
    },
    "alert": {
      "name": "default-alerts",
      "namespace": "default"
    },
    "error": "secrets \"slack-url-foo\" not found"
  }

internal/server/event_server.go Outdated Show resolved Hide resolved
internal/server/event_handlers.go Outdated Show resolved Hide resolved
@darkowlzz darkowlzz added enhancement New feature or request area/alerting Alerting related issues and PRs labels Jun 12, 2023
@darkowlzz
Copy link
Contributor

darkowlzz commented Jun 12, 2023

For other reviewers, I'd like to highlight that the involved object in the log is under a new field called eventInvolvedObject. We can have a discussion if the name is okay or needs to be something else, like just involvedObject, because these are logs from the event-server.
Also, if any other attributes of the incoming event needs to be included in the logger, like severity, reporting controller, reporting instance, etc.

Signed-off-by: Matheus Pimenta <matheuscscp@gmail.com>
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!
I'll update #540 based on this. Thanks @matheuscscp one less new thing to introduce there 🙂 .

@makkes makkes merged commit 3944f2e into fluxcd:main Jun 13, 2023
@matheuscscp matheuscscp deleted the logger-midware branch June 13, 2023 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Alerting related issues and PRs enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants