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

Fix using SemanticLogger#log(severity, message, progname) #2748

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Fix using SemanticLogger#log(severity, message, progname) #2748

merged 1 commit into from
Apr 12, 2023

Conversation

rqz13
Copy link
Contributor

@rqz13 rqz13 commented Apr 5, 2023

SemanticLogger provides interface which is fully compatible with Logger interface
It is possible to use SemanticLogger#add and SemanticLogger#log instance methods on SemanticLogger as it would be just plain Logger
Datadog instrumentation doesn't expect that first argument in method #log might be not SemanticLogger::Log but severity level like Logger::WARN as result fails with error NoMethodError: undefined method 'named_tags' for 2:Integer.

Originally met issue in Rails project, here is specs:

  • rails 6.1.7.3
  • rails_semantic_logger 4.12.0
  • semantic_logger 4.13.0
  • ddtrace 1.4.2

When have configuration for datadog:

Datadog.configure do |conf|
  conf.env = Rails.env
  conf.service = 'my_service'
  conf.tracing.instrument :rails
  conf.tracing.sampling.default_rate = 1.0
end

When have enabled :rails' instrumentation, ':semantic_logger added right away as there is SemanticLogger on project.
Real case scenario is that on project there might be some external gems that may allow to provide logger as part of it's configuration, but may work with logger as it's plain ruby's Logger and use #add or #log methods.
In case of using #log it causes failure because instrumentation prepended and there is no safeguard on first argument log

Example to reproduce having rails with setup datadog to instrument rails and semantic_logger installed:

SemanticLogger['MyService'].log(Logger::WARN, 'some warn')

  SemanticLogger provides interface which is fully compatible with Logger interface
  It is possible to use #add and #log instance methods on SemanticLogger as it would be just plain Logger
  Datadog instrumentation doesn't expect that first argument in method #log might be not SemanticLogger::Log but severity level like Logger::WARN
@rqz13 rqz13 requested a review from a team April 5, 2023 02:05
@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Apr 5, 2023
@TonyCTHsu
Copy link
Contributor

👋 @rqz13 Thanks for submitting this PR!

To be honest, I am not very familiar with SemanticLogger, it might take me some time to learn more about the details and plays around with it before giving proper feedback and code review.

@rqz13
Copy link
Contributor Author

rqz13 commented Apr 5, 2023

@TonyCTHsu thanks for looking into PR.

I'll try explain logic and data flow in more details, hope it would be helpful.
SemanticLogger guarantees compatimility with Logger interface, so it also provides implementation of those methods, add method and log method definitions.

The change I'm introducing based on check of arguments that are passed to log method. It might be SemanticLogger::Log instance when called with "traditional" methods like logger.info('some info') or it might be severity passed like for ruby's Logger logger.log(Logger::INFO, 'some info').
Because of instrumentation prepended into SemanticLogger this check is not performed and cause error, so I've add guard check in instrumentation.
When severity passed as log argument with the guard check it would call execution of SemanticLoggers original log method and also would to the check
Then flow would lead us into add method which would call log_internal
In log_internal method all the args would be processed and SemanticLogger::Log entity would be created and passed again to log method

Now we're back in our instrumentation method but now log argument would be instance of SemanticLogger::Log so the guard check would be skipped in that case and we can inject some extra tags into log.named_tags without failures.

@delner delner added the community Was opened by a community member label Apr 6, 2023
@TonyCTHsu
Copy link
Contributor

👋 @rqz13 , Thanks for the detail elaboration! I am able to reproduce the error and understand the problem! 👍

@TonyCTHsu TonyCTHsu added this to the 1.11.0 milestone Apr 12, 2023
@TonyCTHsu TonyCTHsu merged commit fe92fc1 into DataDog:master Apr 12, 2023
@rqz13 rqz13 deleted the semantic_logger-compatible-logging branch April 12, 2023 17:01
@lloeki lloeki modified the milestones: 1.11.0, 1.11.0.beta1 Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants