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

Merge hash fields recursively for additional fields and child logger #118

Merged
merged 1 commit into from
Feb 20, 2021

Conversation

flexoid
Copy link
Contributor

@flexoid flexoid commented Feb 16, 2021

Hi.

First of all, thanks for a great gem!

When using structured logging, it is very common to have nested fields structure. Even some new standards, something like Elastic Common Schema completely relies on nested fields.

So it is very convenient to have a way to extend log events with additional fields and with child loggers, even if those fields are nested.

Something like this:

logger = Ougai::Logger.new(STDOUT)
logger.with_fields = { process: { pid: 123 } }

module_logger = logger.child(event: { module: "core" } })

class_logger = module_logger.child(event: { dataset: "core.archive" } })

class_logger.info("Item was archived successfully", event: { action: "item-archived" })

and expected:

{"msg": "Item was archived successfully", "process": { "pid": 123 }, "event": {"module": "core", "dataset": "core.archive", "action": "item-archived"} }

I don't think this change will cause any significant performance degradation. Hope it will be merged!

@flexoid
Copy link
Contributor Author

flexoid commented Feb 16, 2021

Looks like codeclimate is offending on increased complexity, but in my opinion, it doesn't feel too complex, and any refactoring will only reduce performance. But let me know if you would prefer some refactoring to be done.

@tilfin tilfin self-requested a review February 17, 2021 01:34
@tilfin
Copy link
Owner

tilfin commented Feb 20, 2021

I don't think it will have that much impact, but I will raise the major version because of the specification change.

Copy link
Owner

@tilfin tilfin left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@tilfin tilfin merged commit cf091f6 into tilfin:master Feb 20, 2021
@tilfin
Copy link
Owner

tilfin commented Feb 21, 2021

released at v2.0.0

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.

2 participants