Skip to content

Discovery new DiscoveryConfigChangeEvent definition - #69626

Open
charlestp wants to merge 1 commit into
charles/prehog-event-keyfrom
charles/discovery-config-changed-event
Open

Discovery new DiscoveryConfigChangeEvent definition#69626
charlestp wants to merge 1 commit into
charles/prehog-event-keyfrom
charles/discovery-config-changed-event

Conversation

@charlestp

Copy link
Copy Markdown
Contributor

This implementation will replace the old DiscoveryConfigEvent. I made a new event since merging into the old event would result in a lot of confusing variations of fields. The new events carries more information that will be useful for product analysis.

  • which client kind made it
  • setup attempt ID so frontend events can be correlated all the way through to backend resource events.
  • separate cloud providers and cloud resource types for easier joining and aggregation.
  • enum types instead of strings

I'm planning to delete the old event in a follow-up PR.

setup_attempt_id will get plumbed through with the frontend events work in a follow-up PR.

Manual Test Plan

Test Environment

Locally

Test Cases

  • New event is emitted with the expected fields

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 94f7593dbb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


// NewDiscoveryConfigChangedEvent builds the event for one change to one
// discovery config.
func NewDiscoveryConfigChangedEvent(ctx context.Context, dc *discoveryconfig.DiscoveryConfig, action prehogv1a.DiscoveryConfigChangeAction) *DiscoveryConfigChangedEvent {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Submit the new event from mutation handlers

Every successful DiscoveryConfig mutation still calls the old emitUsageEvent (lib/auth/discoveryconfig/discoveryconfigv1/service.go:222,270,315,358), and a repo-wide search finds NewDiscoveryConfigChangedEvent only here and in its test. Consequently, tp.discovery.config.changed has no producer, so none of the new action, client, setup-attempt, matcher, or Access Graph data is ever reported; wire this constructor into the create, update, upsert, and delete paths with their request context.

Useful? React with 👍 / 👎.

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

Labels

backport/branch/v18 no-changelog Indicates that a PR does not require a changelog entry size/lg

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant