Add support for Logstash as a valid Agent destination #24305

ph · 2021-03-02T22:29:19Z

This PR fixes an issue to use an logstash output as a monitoring
destination, this correctly uses the type defined in the output to
generate the appropriate output configuration for the monitoring
processes.

Fixes: 22051

What does this PR do?

Why is it important?

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

[ ]

How to test this PR locally

Start logstash with this configuration:

input {
  beats {
    port => 5555
  }
}
output {
  stdout {
    codec => rubydebug
  }
}

Related issues

Use cases

Screenshots

Logs

botelastic · 2021-03-02T22:29:47Z

This pull request doesn't have a Team:<team> label.

elasticmachine · 2021-03-02T23:18:46Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #24305 updated
Start Time: 2021-06-18T20:44:31.792+0000
Duration: 79 min 46 sec
Commit: 8d82e08

Test stats 🧪

Test	Results
Failed	0
Passed	6964
Skipped	16
Total	6980

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	6964
Skipped	16
Total	6980

ph · 2021-03-04T20:24:30Z

@jsvd @roaksoax Hello, I've fixed the problem with Elastic Agent when Logstash was selected as the default destination, this allow logstash to receive both normal data stream document and monitoring document.

I am looking at the e2e testing for this new output is this something we could contribute together?

FYI @mostlyjason @acchen97

roaksoax · 2021-03-09T19:23:32Z

@andsel can you please take over this?

andsel · 2021-03-11T08:21:43Z

Hi @ph great to know about this, I think I could get a copy of this branch, build and test it locally, to check that the connection between Elastic Agent and Logstash is fine.

ph · 2021-03-11T14:08:14Z

/package

ph · 2021-03-11T14:11:24Z

/package

ph · 2021-03-11T14:13:09Z

@andsel I just rebased this branch on top of master, will you take a look at elastic/e2e-testing#364 to automate it? I remember that Logstash had an integration test in the Beats plugin itself so we need to figure out what we do with this change.

andsel · 2021-03-11T15:44:41Z

In Logstash we integration tests that runs Filebeat and Elasticsearch, I think we could add one to have Elastic Agent pushing to Logstash and to Elasticsearch. Did you mean that?

ph · 2021-03-17T14:25:17Z

In Logstash we integration tests that runs Filebeat and Elasticsearch, I think we could add one to have Elastic Agent pushing to Logstash and to Elasticsearch. Did you mean that?

Yes this is exactly what I mean.

elasticmachine · 2021-03-17T14:25:21Z

Pinging @elastic/agent (Team:Agent)

ph · 2021-03-17T14:28:11Z

@michalpristas this is ready for review.

urso · 2021-03-24T15:41:01Z

x-pack/elastic-agent/pkg/agent/application/monitoring_decorator.go

+	} else {
+		typeValue = elasticsearchKey
+
+	}


nit; It seems we have this pattern in more than one place. How about introducing a transpiler.LookupString(ast, key) (contents string, ok bool)?

urso · 2021-03-24T15:41:54Z

x-pack/elastic-agent/pkg/agent/application/monitoring_decorator.go

+	if found {
+		v, ok := t.Value().(*transpiler.StrVal)
+		if !ok {
+			return programsToRun, nil


Shouldn't this be an error? It looks we hit this line if someone did not follow the config format schema. If so we should rather fail instead of continuing with some 'internal' state that is difficult to understand for the user.

This indeed should error, I've tried refactoring in this PR but it became a mess, I will create a followup issue so we can consider this as techdebt and refactor it.

urso · 2021-03-24T15:44:33Z

x-pack/elastic-agent/pkg/agent/application/monitoring_decorator_test.go

@@ -171,6 +171,85 @@ GROUPLOOP:
 	}
 }

+func TestMonitoringToLogstashInjection(t *testing.T) {


A test with an invalid configuration might be nice to have.

mergify · 2021-04-07T16:28:50Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b elastic-agent/add-support-for-logstash upstream/elastic-agent/add-support-for-logstash
git merge upstream/master
git push upstream elastic-agent/add-support-for-logstash

michalpristas · 2021-06-09T08:12:08Z

brought it even with top before review

michalpristas

tested with local logstash and i can see logs/metrics getting in

michalpristas

please revisit getMonitoringSteps

ph · 2021-06-09T14:07:33Z

@michalpristas for getMonitoringSteps can you give me more details I am not familiar with that area of the code?

michalpristas · 2021-06-09T14:33:23Z

in get monitoring steps there's part of the code which extracts elasticsearch output. it is hardcoded to extract elasticsearch output. this extracted output is then passed to generate method which composes configuration for monitoring beats. the output extracted in first step is used and passed forward

beats/x-pack/elastic-agent/pkg/agent/operation/monitoring.go

Line 98 in 383f689

    
           func (o *Operator) getMonitoringSteps(step configrequest.Step) []configrequest.Step {

also in the same file we use hardcoded elasticsearch type on 2 other places

This PR fixes an issue to use an logstash output as a monitoring destination, this correctly uses the type defined in the output to generate the appropriate output configuration for the monitoring processes.

ph · 2021-06-17T20:04:15Z

/package

michalpristas

please fix tests, go vet ./... tells you what's wrong.
other than that code looks good

I think something is off, when I was doing testing.

ph · 2021-06-18T17:29:25Z

OK, I've made a change locally to getMonitoringSteps and currently writing unit tests for it.
@urso For the integration tests, I don't think this can be easily done in this repository. I am taking a look at the e2e repository how I can add a new service to the running one.

the monitoring sidecar

Replace the idiom of using Lookup and type cast into a string by a single method call.

ph · 2021-06-18T20:45:48Z

@michalpristas Can you do another review please using a real logstash instance. I've put a working configuration in the description above. I want more tests to make sure we don't have a regression.

michalpristas

tested with ES and LS outputs, everything seems to be working fine

ph · 2021-06-23T12:51:16Z

Merging this and creating followup issues.

* Add support for Logstash as a valid Agent destination This PR fixes an issue to use an logstash output as a monitoring destination, this correctly uses the type defined in the output to generate the appropriate output configuration for the monitoring processes. (cherry picked from commit 9de1352)

* Add support for Logstash as a valid Agent destination This PR fixes an issue to use an logstash output as a monitoring destination, this correctly uses the type defined in the output to generate the appropriate output configuration for the monitoring processes. (cherry picked from commit 9de1352) Co-authored-by: Pier-Hugues Pellerin <phpellerin@gmail.com>

ph requested a review from michalpristas March 2, 2021 22:29

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Mar 2, 2021

ph added the Team:Elastic-Agent Label for the Agent team label Mar 2, 2021

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Mar 2, 2021

ph added in progress Pull request is currently in progress. needs_team Indicates that the issue/PR needs a Team:* label labels Mar 2, 2021

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Mar 2, 2021

ph self-assigned this Mar 2, 2021

ph mentioned this pull request Mar 2, 2021

[Elastic-Agent] Monitoring should not use logstash output configuration to configure itself #22051

Closed

ph force-pushed the elastic-agent/add-support-for-logstash branch from 16a9288 to 553ff45 Compare March 11, 2021 14:11

ph marked this pull request as ready for review March 17, 2021 14:25

urso reviewed Mar 24, 2021

View reviewed changes

michalpristas approved these changes Jun 9, 2021

View reviewed changes

michalpristas requested changes Jun 9, 2021

View reviewed changes

Add support for Logstash as a valid Agent destination

ecc6140

This PR fixes an issue to use an logstash output as a monitoring destination, this correctly uses the type defined in the output to generate the appropriate output configuration for the monitoring processes.

ph force-pushed the elastic-agent/add-support-for-logstash branch from b6f385c to ecc6140 Compare June 17, 2021 20:03

michalpristas previously approved these changes Jun 18, 2021

View reviewed changes

Refactor the test with the new visibility of structs and methods

0112ac4

ph added 6 commits June 18, 2021 15:14

remove hardcoded Elasticsearch reference into the output generate for

6c15fab

the monitoring sidecar

Make sure everything is configured correctly

7d080f7

Remove not updated comment about configuration

b77bb95

Add a LookupString to the transpiler package

41acb9e

Replace the idiom of using Lookup and type cast into a string by a single method call.

Fix changelog

6d23fb7

Use lookupString instead of lookup

8d82e08

michalpristas approved these changes Jun 22, 2021

View reviewed changes

ph added review backport-v7.14.0 Automated backport with mergify and removed in progress Pull request is currently in progress. labels Jun 23, 2021

ph merged commit 9de1352 into elastic:master Jun 23, 2021

mergify bot mentioned this pull request Jun 23, 2021

[7.x](backport #24305) Add support for Logstash as a valid Agent destination #26442

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Logstash as a valid Agent destination #24305

Add support for Logstash as a valid Agent destination #24305

ph commented Mar 2, 2021 •

edited

Loading

botelastic bot commented Mar 2, 2021

elasticmachine commented Mar 2, 2021 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

ph commented Mar 4, 2021

roaksoax commented Mar 9, 2021

andsel commented Mar 11, 2021

ph commented Mar 11, 2021

ph commented Mar 11, 2021

ph commented Mar 11, 2021

andsel commented Mar 11, 2021

ph commented Mar 17, 2021

elasticmachine commented Mar 17, 2021

ph commented Mar 17, 2021

urso Mar 24, 2021

urso Mar 24, 2021

ph Jun 18, 2021

urso Mar 24, 2021

mergify bot commented Apr 7, 2021

michalpristas commented Jun 9, 2021

michalpristas left a comment

michalpristas left a comment

ph commented Jun 9, 2021

michalpristas commented Jun 9, 2021 •

edited

Loading

ph commented Jun 17, 2021

michalpristas left a comment

ph commented Jun 18, 2021

ph commented Jun 18, 2021

michalpristas left a comment

ph commented Jun 23, 2021

Add support for Logstash as a valid Agent destination #24305

Add support for Logstash as a valid Agent destination #24305

Conversation

ph commented Mar 2, 2021 • edited Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

botelastic bot commented Mar 2, 2021

elasticmachine commented Mar 2, 2021 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

💚 Flaky test report

Test stats 🧪

ph commented Mar 4, 2021

roaksoax commented Mar 9, 2021

andsel commented Mar 11, 2021

ph commented Mar 11, 2021

ph commented Mar 11, 2021

ph commented Mar 11, 2021

andsel commented Mar 11, 2021

ph commented Mar 17, 2021

elasticmachine commented Mar 17, 2021

ph commented Mar 17, 2021

urso Mar 24, 2021

Choose a reason for hiding this comment

urso Mar 24, 2021

Choose a reason for hiding this comment

ph Jun 18, 2021

Choose a reason for hiding this comment

urso Mar 24, 2021

Choose a reason for hiding this comment

mergify bot commented Apr 7, 2021

michalpristas commented Jun 9, 2021

michalpristas left a comment

Choose a reason for hiding this comment

michalpristas left a comment

Choose a reason for hiding this comment

ph commented Jun 9, 2021

michalpristas commented Jun 9, 2021 • edited Loading

ph commented Jun 17, 2021

michalpristas left a comment

Choose a reason for hiding this comment

ph commented Jun 18, 2021

ph commented Jun 18, 2021

michalpristas left a comment

Choose a reason for hiding this comment

ph commented Jun 23, 2021

ph commented Mar 2, 2021 •

edited

Loading

elasticmachine commented Mar 2, 2021 •

edited by jenkins-beats-ci bot

Loading

michalpristas commented Jun 9, 2021 •

edited

Loading