Skip to content

Conversation

@droslean
Copy link
Member

/cc @openshift/test-platform

@Copilot Copilot AI review requested due to automatic review settings October 23, 2025 13:30
@openshift-ci openshift-ci bot requested a review from a team October 23, 2025 13:30
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 23, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new events metrics plugin with a general schema for capturing and storing event data. The implementation adds an eventsPlugin alongside existing metrics plugins to collect events with structured metadata including level, source, locator, and message information.

Key changes:

  • Created a new events plugin with a general event schema supporting Info, Warning, and Error levels
  • Integrated the events plugin into the MetricsAgent lifecycle (initialization, recording, and flushing)
  • Refactored the flush method to use map literal initialization

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
pkg/metrics/events.go Implements the new events plugin with event schema definitions and plugin interface methods
pkg/metrics/metrics.go Integrates the events plugin into MetricsAgent for event recording and output

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

}

func NewEvent(level EventLevel, locator EventLocator, message EventMessage, source string, from, to time.Time) *Event {
return &Event{Level: level, Source: source, From: from, To: to, Locator: locator, Message: message}
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The Timestamp field is not set in the NewEvent constructor. The Event struct includes a Timestamp field (line 29) which remains zero-valued when events are created through this constructor. Consider either setting it in the constructor or documenting that callers must use SetTimestamp separately.

Suggested change
return &Event{Level: level, Source: source, From: from, To: to, Locator: locator, Message: message}
return &Event{
Level: level,
Source: source,
From: from,
To: to,
Locator: locator,
Message: message,
Timestamp: time.Now(),
}

Copilot uses AI. Check for mistakes.
}

func (p *eventsPlugin) Events() []MetricsEvent {
return p.events
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The Events() method returns the slice without mutex protection, which could lead to race conditions if called concurrently with Record(). Consider acquiring the mutex before returning the slice or returning a copy to ensure thread safety.

Suggested change
return p.events
p.mu.Lock()
defer p.mu.Unlock()
eventsCopy := make([]MetricsEvent, len(p.events))
copy(eventsCopy, p.events)
return eventsCopy

Copilot uses AI. Check for mistakes.
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 23, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: droslean

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 23, 2025

@droslean: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/lint d60c704 link true /test lint
ci/prow/breaking-changes d60c704 link false /test breaking-changes
ci/prow/images d60c704 link true /test images

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant