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

[pkg/stanza] Cache event publishers: log warn once per provider #27658

Conversation

pjanotti
Copy link
Contributor

Description:

Cache the publisher event to:

  1. Avoid logging the same error message every time one event from the given source is logged.
  2. Avoid opening and closing the event publisher for every single event.

Link to tracking Issue:

Item 4 described on the investigation for issue #21491.

Testing:

  • Go tests for pkg/stanza and receiver/windowseventlogreceiver on Windows box.
  • Ran the contrib build locally to validate the change.
  • Can't run the full make locally: misspell is failing on Windows because the command line is too long.

Documentation:

Let me know if changing the severity of the log message requires a changelog update.

@pjanotti pjanotti requested review from djaglowski and a team as code owners October 12, 2023 22:28
@pjanotti pjanotti changed the title Cache event publishers: log warn once per provider [pkg/stanza] Cache event publishers: log warn once per provider Oct 12, 2023
@crobert-1 crobert-1 added the Run Windows Enable running windows test on a PR label Oct 12, 2023
@djaglowski
Copy link
Member

@pjanotti, thanks for the work on this.

You appear to have a lot more expertise than I on the details of WEL. Are you interested in being a code owner on this component? (I'm happy to stay on to help with general issues relating to pkg/stanza, etc.)

@pjanotti
Copy link
Contributor Author

@djaglowski, yes, I'm interested on that. I'm planning to focus on Windows related stuff in general. The package is pretty big and it will take some time for me to get used to it, but, let's get started.

@djaglowski
Copy link
Member

Thanks @pjanotti. Please submit a PR to add yourself as an owner.

This PR looks good to me. I think we just need a changelog.

@djaglowski djaglowski merged commit 784514f into open-telemetry:main Oct 13, 2023
87 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 13, 2023
djaglowski pushed a commit that referenced this pull request Oct 13, 2023
**Description:**
Adding myself as owner of `windowseventlogreceiver` per invite
#27658 (comment)

cc @djaglowski
@pjanotti pjanotti deleted the log-warning-when-publisher-not-available branch October 13, 2023 18:35
jorgeancal pushed a commit to jorgeancal/opentelemetry-collector-contrib that referenced this pull request Oct 15, 2023
…-telemetry#27658)

**Description:**

Cache the publisher event to:

1. Avoid logging the same error message every time one event from the
given source is logged.
2. Avoid opening and closing the event publisher for every single event.

**Link to tracking Issue:**

[Item 4 described on the
investigation](open-telemetry#21491 (comment))
for issue open-telemetry#21491.

**Testing:**

* Go tests for `pkg/stanza` and `receiver/windowseventlogreceiver` on
Windows box.
* Ran the contrib build locally to validate the change.
* Can't run the full make locally: misspell is failing on Windows
because the command line is too long.

**Documentation:**

Let me know if changing the severity of the log message requires a
changelog update.
jorgeancal pushed a commit to jorgeancal/opentelemetry-collector-contrib that referenced this pull request Oct 15, 2023
**Description:**
Adding myself as owner of `windowseventlogreceiver` per invite
open-telemetry#27658 (comment)

cc @djaglowski
JaredTan95 pushed a commit to openinsight-proj/opentelemetry-collector-contrib that referenced this pull request Oct 18, 2023
…-telemetry#27658)

**Description:**

Cache the publisher event to:

1. Avoid logging the same error message every time one event from the
given source is logged.
2. Avoid opening and closing the event publisher for every single event.

**Link to tracking Issue:**

[Item 4 described on the
investigation](open-telemetry#21491 (comment))
for issue open-telemetry#21491.

**Testing:**

* Go tests for `pkg/stanza` and `receiver/windowseventlogreceiver` on
Windows box.
* Ran the contrib build locally to validate the change.
* Can't run the full make locally: misspell is failing on Windows
because the command line is too long.

**Documentation:**

Let me know if changing the severity of the log message requires a
changelog update.
JaredTan95 pushed a commit to openinsight-proj/opentelemetry-collector-contrib that referenced this pull request Oct 18, 2023
**Description:**
Adding myself as owner of `windowseventlogreceiver` per invite
open-telemetry#27658 (comment)

cc @djaglowski
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this pull request Nov 12, 2023
…-telemetry#27658)

**Description:**

Cache the publisher event to:

1. Avoid logging the same error message every time one event from the
given source is logged.
2. Avoid opening and closing the event publisher for every single event.

**Link to tracking Issue:**

[Item 4 described on the
investigation](open-telemetry#21491 (comment))
for issue open-telemetry#21491.

**Testing:**

* Go tests for `pkg/stanza` and `receiver/windowseventlogreceiver` on
Windows box.
* Ran the contrib build locally to validate the change.
* Can't run the full make locally: misspell is failing on Windows
because the command line is too long.

**Documentation:**

Let me know if changing the severity of the log message requires a
changelog update.
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this pull request Nov 12, 2023
**Description:**
Adding myself as owner of `windowseventlogreceiver` per invite
open-telemetry#27658 (comment)

cc @djaglowski
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg/stanza Run Windows Enable running windows test on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants