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.

Sign up for GitHub

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

Jump to bottom

[Metricbeat] Add STAN (NATS streaming) module #14839

Merged
merged 36 commits into from
Dec 13, 2019
Merged

[Metricbeat] Add STAN (NATS streaming) module #14839

merged 36 commits into from
Dec 13, 2019

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Nov 28, 2019 •
edited
Loading

This PR cherry-picks the commits of @devon-kim
from 7.2...devon-kim:ops-1844-stan-metricbeat so as to be merged to beats/master as part of #14629.

cc: @exekias

Manual testing

  1. Build the Docker image: docker build -t stan_test . (run this command under x-pack/metricbeat/module/stan/_meta)
  2. Start STAN server: docker run -p 8222:8222 stan_test
  3. Enable the module and run Metricbeat.
    (Note that default fetching period is 60secs, so this might should be changed for the testing)

@ChrsMark ChrsMark added in progress Pull request is currently in progress. module Metricbeat Metricbeat [zube]: In Progress labels Nov 28, 2019
@ChrsMark ChrsMark requested a review from a team as a code owner November 28, 2019 11:38
@ChrsMark ChrsMark self-assigned this Nov 28, 2019
houndci-bot
houndci-bot reviewed Nov 28, 2019
View reviewed changes
metricbeat/module/stan/channels/data.go Outdated Show resolved Hide resolved
metricbeat/module/stan/channels/data.go Outdated Show resolved Hide resolved
metricbeat/module/stan/channels/data.go Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Nov 28, 2019
View reviewed changes
metricbeat/module/stan/subscriptions/data.go Outdated Show resolved Hide resolved
metricbeat/module/stan/subscriptions/data.go Outdated Show resolved Hide resolved
metricbeat/module/stan/subscriptions/data.go Outdated Show resolved Hide resolved
metricbeat/module/stan/channels/data.go Outdated Show resolved Hide resolved
metricbeat/module/stan/channels/data.go Outdated Show resolved Hide resolved
metricbeat/module/stan/channels/data.go Outdated Show resolved Hide resolved
@elasticcla
Copy link

Hi @ChrsMark, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@ChrsMark ChrsMark added the Team:Integrations Label for the Integrations team label Nov 28, 2019
@ChrsMark ChrsMark force-pushed the stan_mb_module branch 2 times, most recently from f5e9331 to 4d9ccfc Compare November 28, 2019 12:50
@ChrsMark ChrsMark force-pushed the stan_mb_module branch 2 times, most recently from e4b2602 to f9ccafe Compare December 2, 2019 14:52
fearful-symmetry
fearful-symmetry reviewed Dec 2, 2019
View reviewed changes
metricbeat/module/stan/channels/channels_test.go Outdated
absPath, _ := filepath.Abs("./_meta/test/")

response, _ := ioutil.ReadFile(absPath + "/channels.json")
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth moving some of the setup to another function?

metricbeat/module/stan/channels/data.go Outdated
if err != nil {
return mb.Event{}, err
}
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're checking the same error twice here, it looks like?

metricbeat/module/stan/channels/data.go Outdated
if evt, err = eventMapping(chWrapper); err != nil {
r.Error(errors.Wrap(err, "failure to map channel to its schema"))
}
if !r.Event(evt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's been some debate about this. The only time r.Event will return false is when the underlying channel is closed and metricbeat is shutting down. I believe we decide that if we check this in a loop (which we should) It shouldn't be reported as an error and we should just return.

devon-kim and others added 6 commits December 2, 2019 17:27
@ChrsMark
improvements
c190bb1 
Signed-off-by: chrismark <[email protected]>
@ChrsMark
Copy link
Member Author

SGTM

not requesting changes but commenting for your consideration

Thanks for the suggestions @odacremolbap ! Updates were pushed.

@ChrsMark ChrsMark added test-plan Add this PR to be manual test plan v7.6.0 labels Dec 11, 2019
@ChrsMark
fix location of Dockefile
33e986e 
Signed-off-by: chrismark <[email protected]>
@ChrsMark
Copy link
Member Author

ChrsMark commented Dec 12, 2019 •
edited
Loading

pinging @jsoriano @exekias about the very latest commit. This was needed so as to prevent Travis from giving a timeout while waiting for output. Do you think this could be included? Or any other ideas on how we could unblock this PR?

@exekias
Copy link
Contributor

exekias commented Dec 12, 2019

As discussed offline, let's try to make mage log the output of go test to avoid this problem

@ChrsMark
Make use travis_wait
82addd4 
Signed-off-by: chrismark <[email protected]>
@ChrsMark
Enable testing output
a4bd16d 
Signed-off-by: chrismark <[email protected]>
@ChrsMark
Copy link
Member Author

ChrsMark commented Dec 13, 2019 •
edited
Loading

Problems with Travis were resolved with the change @jsoriano proposed in #15083. The changes we applied here directly. Merging this.

@ChrsMark ChrsMark merged commit 10c741a into elastic:master Dec 13, 2019
@ChrsMark ChrsMark mentioned this pull request Dec 13, 2019
Closed
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Dec 13, 2019
@ChrsMark
[Metricbeat] Add STAN (NATS streaming) module (elastic#14839)
c315812 
(cherry picked from commit 10c741a)
@ChrsMark ChrsMark mentioned this pull request Dec 13, 2019
Merged
ChrsMark added a commit that referenced this pull request Dec 16, 2019
@ChrsMark
Cherry-pick #14839 to 7.x: [Metricbeat] Add STAN (NATS streaming) mod…
00c3c76 
…ule (#15092)
@odacremolbap odacremolbap self-assigned this Jan 16, 2020
@odacremolbap odacremolbap added the test-plan-ok This PR passed manual testing label Jan 20, 2020
@jsoriano jsoriano mentioned this pull request Jan 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devon-kim devon-kim devon-kim left review comments

@kaiyan-sheng kaiyan-sheng kaiyan-sheng left review comments

@fearful-symmetry fearful-symmetry fearful-symmetry left review comments

@houndci-bot houndci-bot houndci-bot left review comments

@mtojek mtojek mtojek approved these changes

@odacremolbap odacremolbap odacremolbap approved these changes

Assignees

@odacremolbap odacremolbap

Labels
Metricbeat Metricbeat module review Team:Integrations Label for the Integrations team test-plan Add this PR to be manual test plan test-plan-ok This PR passed manual testing v7.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@ChrsMark @elasticcla @devon-kim @exekias @houndci-bot @fearful-symmetry @odacremolbap @mtojek @kaiyan-sheng @andresrc