-
Notifications
You must be signed in to change notification settings - Fork 6
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
Change scoping of temporary tags #22
base: master
Are you sure you want to change the base?
Conversation
@@ -87,13 +87,15 @@ def log(additional_tags = []) | |||
event = build_event(@source, tags) | |||
@output.dump(event) | |||
end | |||
tags.clear |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we want to clear the tags in this method (check "food for thoughts" bellow for another alternative) it should be in an ensure
block. By the way at the moment if an exception occurs we don't dump the event. Is it expected ?
FTR: the store is not cleared which means that it leaks between threads.
The fact we clear it before it's used again prevents using values set by a previous request though (unless someone passes a string to the log method for instance, in which case exception is logged but store is not cleared [1])
IMO the store should be cleared after yielding in an ensure
block.
Food for thoughts:
The fact that we clear the tags in the log method prevents having a tag for multiple logs. For instance let's say we have a batch that takes quite a lot of time and we would like to monitor a few steps in realtime. If the batch can be executed from different location we could have a different tag per location (api, cron ... ).
In a middleware:
def call(env)
Peastash.with_instance(:custom_instance).tags << 'api'
@app.call(env)
end
end
In service class :
Peastash.with_instance(:custom_instance).log { |instance| instance.store[:message] = 'step1' }
Peastash.with_instance(:custom_instance).log { |instance| instance.store[:message] = 'step2' }
Peastash.with_instance(:custom_instance).log { |instance| instance.store[:message] = 'step3' }
Peastash.with_instance(:custom_instance).log { |instance| instance.store[:message] = 'step4' }
I think a good inspiration could be to use a stack as in rails tagged logging [2]
[1]
(byebug) Peastash.with_instance(:test).store.merge!(key: 'value')
{:key=>"value"}
(byebug) Peastash.with_instance(:test).log('should_be_an_array') { puts Peastash.with_instance(:test).store }
#<TypeError: no implicit conversion of String into Array>
...
{:key=>"value"}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify a bit what I said I think we should have the yield
inside a begin/ensure block.
We should clear the store and the tags in the ensure
block to avoid leaks to other requests. I think the dump should also be part of the ensure
block. I cannot think of a scenario where we don't want to dump when an error occurs.
WDYT?
The food for thought section is a suggestion that is not related to this PR. I think it's ok to clear tags that are defined inside a log
method in the context of the rake middleware for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The store is cleared every time the log method is called.
The usecase where the same Peastash instance is calling log
in a nested fashion isn't supported, and I can't really think of a reason to support it.
However, I hould make it explicit in the documentation at least.
With that in mind and the fact that the store and the tags are reset at the beginning of every log
block, I don't that store leakage is a concern. The store
construct is thread-bound, so using the same instance at the same time in multiple threads is safe.
Regarding exceptions happening in the .log
block, indeed this will result in no logging. This is to keep Peastash as neutral as possible, and not interfere with exception management of the apps that peastash is embedded in.
In a previous company where I used Peastash in production, performing the logging even in case of exceptions caused more confusion than it solved, so the gem was kept that way.
Solves #20.
This PR introduces two changes:
#log
blockPeastash
instance.