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

Loki Logstash Plugin #1822

Merged
merged 30 commits into from
Jul 17, 2020
Merged

Conversation

adityacs
Copy link
Contributor

@adityacs adityacs commented Mar 19, 2020

What this PR does / why we need it:
Logstash plugin for Loki

Which issue(s) this PR fixes:
Fixes #1192

Checklist

  • Documentation added
  • Tests updated

@adityacs
Copy link
Contributor Author

@cyriltovena @slim-bean Need your thoughts and guidance on this

@cyriltovena
Copy link
Contributor

cyriltovena commented Mar 19, 2020

Can you remove the .class and .gem files, basically all artefacts. Feel free to add an .gitignore for that folter.

logstash/build.gradle Outdated Show resolved Hide resolved
logstash/build.gradle Outdated Show resolved Hide resolved
@cyriltovena
Copy link
Contributor

Have you considered writing the plugin only in ruby ? I think it would make our life easier since we also have a ruby plugin for fluentd.

@adityacs
Copy link
Contributor Author

@cyriltovena Ruby is new to me. It would take little more time for me to write the plugin in Ruby. I totally agree that it would be lot easier to maintain the code since we already have fluentd in Ruby.

What do you think? If you say let's stick to Ruby, let's go with Ruby. I will close this and create new PR.

@slim-bean
Copy link
Collaborator

Hey @adityacs, firstly can't thank you enough for all your contributions lately. You are a pleasure to work with and we really appreciate everything you are doing!!

I think in this case Ruby would be preferred as adding the java toolchain I think is adding another amount of complication we may not want. It looks like most other logstash plugins are ruby based.

@adityacs
Copy link
Contributor Author

@slim-bean Yeah Ruby would be better. I just took an easy route since I am familiar with Java 😜

I will work on the Ruby plugin and update this PR.

@cyriltovena
Copy link
Contributor

I'm not the best but I've been involved a lot lately with the fluentd plugin in Ruby, so don't hesitate to ask question on #loki-dev by pinging me or @slim-bean.

@adityacs
Copy link
Contributor Author

Sure @cyriltovena

@adityacs
Copy link
Contributor Author

@slim-bean @cyriltovena Thanks for the feedback. It's a great experience to work with you guys.
Thanks again for all the guidance.

@adityacs adityacs force-pushed the loki_logstash_plugin branch 2 times, most recently from 4754a74 to 9b1d683 Compare March 26, 2020 14:07
@adityacs
Copy link
Contributor Author

adityacs commented Mar 26, 2020

Pushed plugin with Ruby code. Have to add add few tests.

@adityacs adityacs force-pushed the loki_logstash_plugin branch 4 times, most recently from 92b9c50 to 6acd391 Compare March 27, 2020 13:24
@adityacs
Copy link
Contributor Author

@cyriltovena @slim-bean Have added tests and also updated the docs. Kindly requesting you guys to review this

Also, let me know if you are able to build and test the plugin.

@codecov-io
Copy link

codecov-io commented Mar 27, 2020

Codecov Report

Merging #1822 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1822      +/-   ##
==========================================
- Coverage   64.75%   64.72%   -0.03%     
==========================================
  Files         122      122              
  Lines        9271     9271              
==========================================
- Hits         6003     6001       -2     
- Misses       2853     2854       +1     
- Partials      415      416       +1     
Impacted Files Coverage Δ
pkg/promtail/targets/tailer.go 76.13% <0.00%> (-2.28%) ⬇️

logstash/LICENSE Outdated
@@ -0,0 +1,202 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

@slim-bean do we need to have a license ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No separate license, this would fall under the license for the project itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove

@cyriltovena
Copy link
Contributor

Can you remove binary artifacts. Feel free to add a .gitignore to this new folder if required.

data_labels = lbls.merge(@external_labels)

entry_hash = {
"ts" => event.get("@timestamp").to_i * (10**9) + Time.new.nsec,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This converts the timestamp to nano seconds

Copy link
Contributor

Choose a reason for hiding this comment

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

I have doubts about the new here, may be a test since we know what timestamp are using logstash ;)

"line" => event.get("message").to_s
}

@Channel.go do
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t think you need a go here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required. Will remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually Channel.go is required here. Since we are using unbuffered channel(i.e without specifying capacity), if we don't use Channel.go it hangs there without proceeding with the code

Copy link
Contributor

@cyriltovena cyriltovena Mar 31, 2020

Choose a reason for hiding this comment

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

Hanging is fine, if it is hanging it is probably waiting for the previous batch to be sent.

If you keep it this way, it means for each event, (million of them) create a go routine. That can be huge for the system.

You may want to use buffered but I think it's better to wait, this also signal, don't send more we're busy to the plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, got your point. Will make the change

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Comment on lines +40 to +43
git clone git@github.com:elastic/logstash.git
cd logstash
git checkout tags/v7.6.2
export LOGSTASH_PATH=`pwd`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice 👍

@adityacs
Copy link
Contributor Author

We are pending with Makefile changes to build this plugin. I will take care of this

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
event.to_hash.each { |key,value|
next if key.start_with?('@')
next if value.is_a?(Hash)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should consider the nested fields as well right? Any reason to skip it?

Copy link
Contributor

Choose a reason for hiding this comment

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

They can already create new field out of current field if they want to.

I think logstash is good enough for managing fields that we shouldn't try to do anything.

adityacs and others added 2 commits July 15, 2020 16:49
Now testing/writing docs and charts.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena
Copy link
Contributor

One sec I've just finished the code.

…loki_logstash_plugin

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena cyriltovena merged commit 4c60907 into grafana:master Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keepalive An issue or PR that will be kept alive and never marked as stale. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logstash plugin
8 participants