-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add pipeline.id to log lines #11075
Add pipeline.id to log lines #11075
Conversation
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.
@andsel - this is awesome.
Tested locally, and this seems to work well for both ruby and java execution engines.
Ideally, I think I'd like to see an integration test, along the lines of the slowlog_spec
to ensure that the pipeline.id is output with a minimal config (which could then potentially be extended when we come up with more use cases when we improve logging)
b8ca006
to
c9ff6fd
Compare
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.
Couple of config nitpicks, and on the name of the test.
@ls.spawn_logstash("-w", "1" , "-e", config) | ||
@ls.wait_for_logstash | ||
sleep 2 until @ls.exited? | ||
pipelinelog_file = "#{temp_dir}/logstash-slowlog-plain.log" |
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.
This should be the plain.log
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.
Actually the name is logstash-plain.log
and checks the plain output file
it "should write logs to a new dir" do | ||
settings = { | ||
"path.logs" => temp_dir, | ||
"slowlog.threshold.warn" => "500ms" |
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.
slowlog.threshold.warn
is unnecessary, as we are not testing the slowlog
count => 4 | ||
} | ||
} | ||
filter { |
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.
This sleep filter is unnecessary (unless you were planning on adding a test that filters are logging the pipeline name correctly)
let(:temp_dir) { Stud::Temporary.directory("logstash-pipelinelog-test") } | ||
let(:config) { @fixture.config("root") } | ||
|
||
it "should write logs to a new dir" do |
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.
Test name should be more descriptive of the test.
c9ff6fd
to
9269a89
Compare
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.
LGTM
3267456
to
cda592f
Compare
this is fairly recent - since 7.4 (added in elasticGH-11075) there's a risk plugins would assume ThreadContext to exist or collide the 'global' constant - usually best to import where the Java class actually gets used ...
this is fairly recent - since 7.4 (added in elasticGH-11075) there's a risk plugins would assume ThreadContext to exist or collide the 'global' constant - usually best to import where the Java class actually gets used ...
Add tag fish with pipeline.id to log lines