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

[receiver/k8sobjects] Add ability to exclude deleted updates for watched objects #26042

Merged
merged 12 commits into from
Sep 5, 2023

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Aug 22, 2023

Description:
Adds a new configuration option that allows excluding DELETED updates for watched objects.

Link to tracking Issue:
Closes #24904

Testing:
New unit tests and tested locally

Documentation:
Updated the readme

@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Aug 22, 2023

I noticed this receiver is using the old style of config testing which makes it difficult to add new scenarios. I plan to submit another PR to update the testing framework: #26078

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

The PR description says the README has been updated, but it's not shown in the changes. Could you add the updated README for review?

@TylerHelmuth TylerHelmuth marked this pull request as ready for review August 24, 2023 15:42
@TylerHelmuth TylerHelmuth requested a review from a team August 24, 2023 15:42
@@ -50,6 +50,7 @@ the K8s API server. This can be one of `none` (for no auth), `serviceAccount`
- `label_selector`: select objects by label(s)
- `field_selector`: select objects by field(s)
- `interval`: the interval at which object is pulled, default 60 minutes. Only useful for `pull` mode.
- `exclude_deleted`: whether the object's deleted updates should be dropped. Only usable in `watch` mode.
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to introduce a config for deleted events only? Shouldn't we have a generic configuration to limit the set of watched events?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, we could make this a generic list of accepted types, which would default to ADDED, UPDATED, and DELETED.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does k8s allow for custom types returned for a watch? I wouldn't want to miss any that aren't the three I mentioned.

Copy link
Member

Choose a reason for hiding this comment

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

Does k8s allow for custom types returned for a watch? I wouldn't want to miss any that aren't the three I mentioned.

I'm not sure. @jinja2, do you know by chance?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe the best would be to have it be an exclude list and if it is empty then we exclude nothing. Then we don't have to worry about guessing what is allowed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok so we can actually use the predefined EventType

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to allow excluding any watch type.

@TylerHelmuth
Copy link
Member Author

running make gotidy cause some changes: #26314

.chloggen/k8sobjects-filter-delete.yaml Outdated Show resolved Hide resolved
require.NotNil(t, r)
require.NoError(t, r.Start(ctx, componenttest.NewNopHost()))

time.Sleep(time.Millisecond * 100)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps an assert.Never would be better here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The unit tests definitely need some work, but I'd prefer to do it in a separate PR.

@jpkrohling jpkrohling merged commit 930b421 into open-telemetry:main Sep 5, 2023
91 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 5, 2023
@TylerHelmuth TylerHelmuth deleted the k8sobjects-filter-delete branch September 5, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[receiver/k8sobjects] Allow specifying whether DELETE actions should be ingested during a watch
6 participants