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

log: add caller plugin-id about emitting error events #4632

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daipom
Copy link
Contributor

@daipom daipom commented Sep 11, 2024

Which issue(s) this PR fixes:

What this PR does / why we need it:
Add caller plugin-id to warning logs about emitting error events.

It would be helpful if we could know what plugin emitted the error event.

We need to care about the compatibility.
This signature change would not break compatibility.

However, I'm concerned that caller_plugin_id has a race condition, although I don't confirm it.
It looks to me that the id can be another plugin-id running concurrently...
It is not the issue with this fix, it is the issue of the existing implementation.

Docs Changes:
Not needed.

Release Note:
Add plugin-id to warning logs about emitting error events.

Example:
From #4567.

<source>
  @type sample
  @id id_sample_not_json
  sample {"data":"this is not json}"}
  tag sample.json
</source>

<filter sample.json>
  @type parser
  @id id_not_json
  key_name data
  <parse>
    @type json
  </parse>
</filter>

<match sample.**>
  @type stdout
</match>
2024-09-11 15:26:26 +0900 [info]: #0 fluentd worker is now running worker=0
2024-09-11 15:26:27 +0900 [warn]: #0 dump an error event:
 error_class=Fluent::Plugin::Parser::ParserError
 error="pattern not matched with data 'this is not json}'"
 location=nil tag="sample.json"
 time=2024-09-11 15:26:27.040657722 +0900
 plugin_id="id_not_json" ### <= This info is added by this PR ###
 record={"data"=>"this is not json}"}
...

It would be helpful if we could know what plugin emitted the
error event.

fluent#4567

We need to care about the compatibility.
This signature change would not break compatibility.

However, I'm concerned that `caller_plugin_id` has a race
condition, although I don't confirm it.
It looks to me that the id can be another plugin-id running
concurrently...
It is not the issue with this fix, it is the issue of the
existing implementation.

Signed-off-by: Daijiro Fukuda <[email protected]>
@daipom
Copy link
Contributor Author

daipom commented Sep 11, 2024

However, I'm concerned that caller_plugin_id has a race condition, although I don't confirm it.
It looks to me that the id can be another plugin-id running concurrently...

def router
@_event_emitter_used_actually = true
if @_event_emitter_lazy_init
@router = @primary_instance.router
end
if @router.respond_to?(:caller_plugin_id=)
@router.caller_plugin_id = self.plugin_id
end
@router
end

@kenhys
Copy link
Contributor

kenhys commented Sep 13, 2024

As error.backtrace does not have enough information in this context, so current approach seems reasonable.
(No need to force 3rdparty plugins to fix it)

Copy link

This PR has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this PR will be closed in 7 days

@github-actions github-actions bot added the stale label Oct 13, 2024
@daipom daipom removed the stale label Oct 15, 2024
@daipom daipom added this to the v1.18.0 milestone Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error event logs should include plugin id
2 participants