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

Use Log#emit and Log::Metadata #573

Merged
merged 2 commits into from
Jun 21, 2024
Merged

Use Log#emit and Log::Metadata #573

merged 2 commits into from
Jun 21, 2024

Conversation

carlhoerberg
Copy link
Member

@carlhoerberg carlhoerberg commented Sep 13, 2023

Saves Log instances and uses Metadata instead, somewhat repetitive but the "native" way.

@carlhoerberg carlhoerberg requested a review from spuun September 13, 2023 13:12
@baelter
Copy link
Member

baelter commented Sep 14, 2023

Can't we wrap this with our own Log, something like

class LavinMQ::Log < ::Log
  property metadata
  def debug
     msg = yield
     super.debug &.emit msg, metadata
  end
  ...
end

Log = LavinMQ::Log.for(self)
Log.metadata = ...
Log.debug { "yada" }

?

Copy link
Member

@spuun spuun left a comment

Choose a reason for hiding this comment

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

Maybe the EntityLog in #502 is over-engineered with its expand methods and stuff should be more explicit, but i think using a Struct to wrap the Log instance and @metadata is nice so you can use the same syntax?

@spuun
Copy link
Member

spuun commented Sep 14, 2023

Can't we wrap this with our own Log, something like

class LavinMQ::Log < ::Log
  property metadata
  def debug
     msg = yield
     super.debug &.emit msg, metadata
  end
  ...
end

Log = LavinMQ::Log.for(self)
Log.metadata = ...
Log.debug { "yada" }

?

The EntityLog here does that:
https://github.com/cloudamqp/lavinmq/blob/logging-sources/src/lavinmq/logging.cr

Saves Log instances and uses Metadata instead, repetetive but better
than the alternative
@carlhoerberg
Copy link
Member Author

Getting rid of @log that we might think be a memory leak. And doesn't do any monkey patching of anything, uses the "native" way of logging metadata with Log.

@carlhoerberg carlhoerberg merged commit f5b8cfb into main Jun 21, 2024
23 of 25 checks passed
@carlhoerberg carlhoerberg deleted the log-metadata branch June 21, 2024 00:31
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.

None yet

3 participants