Add Pipeline registration and add default pipelines. #1258

simitt · 2018-08-03T11:50:03Z

implements #1032

This PR introduces pipeline registration.

The default pipeline registered in ES when registration.ingestion.pipeline.enabled=true is the User Agent Pipeline, depending on the Ingest User Agent Plugin. As this plugin is not pre-installed in a standard ES download, I disabled the plugin registration by default.
As registration.ingest.pipeline.enabled=true APM Server loads all pipeline definitions from ingest/pipeline/definition.json and sends them to the ES API endpoint.

Generally the pipeline registration works for the run and the setup cmd, by registering to the according libbeat callbacks.

depends on #1256 to be merged first [UPDATE: merged]
The changes in the vendor folder will be removed, once elastic/beats#7851 is merged. [UPDATE: removed, as #1256 was merged]

simitt · 2018-08-07T10:00:56Z

@elastic/apm-server anyone up for a review?

jalvz · 2018-08-07T12:59:05Z

_beats/libbeat/scripts/Makefile

@@ -450,4 +450,3 @@ release: mage

 .PHONY: snapshot
 snapshot: mage
-	@SNAPSHOT=true mage package


yes! All changes in _beats and vendor will be replaced once the updated libbeat code is merged in.

jalvz · 2018-08-07T13:03:52Z

_meta/beat.yml

+  # Pipeline definitions are registered at the configured Elasticsearch instances.
+  # Registering pipelines therefore requires to have `output.elasticsearch` enabled and configured.
+  #
+  # If registering pipelines is disabled, you can still configure `output.elasticsearch.pipelines`


probably because i never looked into pipelines, this comment is hard to understand for me: is the case that output.elasticsearch.pipelines is the same as register.ingest.pipeline for existing documents? or why do you mention it here?

jalvz · 2018-08-07T13:08:00Z

_meta/beat.yml

@@ -374,8 +393,14 @@ output.elasticsearch:
      when.contains:
        processor.event: "onboarding"

-  # Optional ingest node pipeline. By default no pipeline will be used.
-  #pipeline: ""
+  # Ingest node pipelines are applied when writing documents to Elasticsearch.


reading this i am not clear if i need to uncomment #pipelines: - pipeline: "apm_user_agent" or not...

simitt · 2018-08-08T08:14:46Z

Thanks for the review @jalvz , I updated the description.

jalvz · 2018-08-08T08:18:19Z

ingest/pipeline/register.go

+)
+
+func RegisterPipelines(esClient *es.Client, overwrite bool, path string) error {
+	pipelines, err := loadPipelinesFromJSON(path)


is this path being resolved correctly with deb/yum/etc packaging?

ok, i saw now that you are packing the json file with mage 👍
do you want me to test that or are you confident with it?

I tested it only with the tar package so far, but can do more testing.

i think eg linux packaging is interesting because assets would end in another directory/etc/something/something i presume?
lmk if you want any help testing that

If you are up for testing - much appreciated, otherwise I'll do I'll later on.

sure, i can test that 👍

jalvz · 2018-08-08T08:22:00Z

ingest/pipeline/register.go

+		if overwrite || !exists {
+			_, _, err := esClient.Connection.CreatePipeline(p.Id, nil, p.Body)
+			if err != nil {
+				logp.Debug("pipelines", "Pipeline registration failed for %s.", p.Id)


this is the deprecated logging package, should be logp.NewLogger(selector).Debugf...

jalvz · 2018-08-08T09:56:59Z

apm-server.yml

+  # You can manually register pipelines, or use this configuration option to ensure
+  # pipelines are loaded and registered at the configured Elasticsearch instances.
+  # Automatic pipeline registration requires 
+  # * `output.elasticsearch` to be enabled and configured.


thanks for updating the docs, much better now!
so there are 2 dependent settings, in output.elasticsearch.pipelines you refer to apm-server.register.ingest.pipeline, but not the other way around.

would be great to add another line like

output.elasticsearch.pipelines contains "apm_user_agent" or similar

I refer to them both ways, see

line 120: (2) applying a pipeline during data ingestion (see output.elasticsearch.pipelines)

line 400: # (1) ensure pipelines are registered in Elasticsearch, see apm-server.register.ingest.pipeline

missed L120, sorry

jalvz · 2018-08-08T09:57:22Z

CHANGELOG.asciidoc

@@ -13,6 +13,7 @@ https://github.com/elastic/apm-server/compare/6.4\...master[View commits]
 === Added

 - Provide basic server information at `/` {pull}1197[1197]
+- Add pipeline registration and pipeline usage {pull}[]


you can update this now

simitt · 2018-08-08T09:58:46Z

jenkins, run package tests please

jalvz · 2018-08-08T09:59:53Z

ingest/pipeline/register_test.go

+	require.NoError(t, err)
+	esClient := &esClients[0]
+	_, current, _, _ := runtime.Caller(0)
+	path := filepath.Join(filepath.Dir(current), "..", "..", "ingest", "pipeline", "definition.json")


not important, but i'd try to reuse functions in tests/loader.go for these things

jalvz · 2018-08-08T10:02:04Z

magefile.go


 		case mage.DMG:
 			mage.Packages = append(mage.Packages[:idx], mage.Packages[idx+1:]...)
+
+		default:
+			panic(errors.Errorf("unhandled package type: %v", pkgType))


why should packaging ever panic, a log is not enough?
if adding a default, would make sense to drop the case mage.DMG then?

The case should not be hit, but if additional packages are added to the package.yml, where we don't have a case defined, then we would be notified with this.

jalvz · 2018-08-08T10:03:05Z

tests/system/test_integration.py

@@ -519,3 +520,41 @@ def test_metric_doc(self):
        expected_type = "scaled_float"
        actual_type = mappings[self.index_name]["mappings"]["doc"]["system.process.cpu.total.norm.pct"]["mapping"]["pct"]["type"]
        assert expected_type == actual_type, "want: {}, got: {}".format(expected_type, actual_type)
+
+
+class PipelineRegisterTest(ElasticTest):


jalvz

(looks good to / works for) me, GJ!

simitt · 2018-08-09T08:51:12Z

Thanks @jalvz for testing the packages, I updated the file handling and tested it.

simitt · 2018-08-10T13:11:00Z

jenkins, test this please.

simitt · 2018-08-10T13:11:22Z

jenkins, retest this please

simitt · 2018-08-10T13:27:30Z

jenkins, retest this please

simitt · 2018-08-13T07:53:37Z

@jalvz as I made several changes after your approval, could you quickly re-review please?

jalvz · 2018-08-13T08:23:06Z

tests/system/apmserver.py

@@ -66,6 +66,13 @@ def setUp(self):
        super(ServerSetUpBaseTest, self).setUp()
        shutil.copy(self._beat_path_join("fields.yml"), self.working_dir)

+        pipeline_dir = os.path.join("ingest", "pipeline")


a comment about why are you doing this so we remember in six months would be nice 👍

jalvz · 2018-08-13T08:23:54Z

i was up to date with everything except last commit, lgtm 👍
thanks for this

* Disable pipeline registration and application by default. * If enabled, register default pipelines on APM Server `run` or `setup`. * Add user_agent pipeline as default pipeline. * Add system tests and log entries. * Add pipeline definition to packaging. implements elastic#1032

graphaelli · 2018-08-28T16:31:00Z

tests/system/test_integration.py

@@ -507,7 +508,7 @@ def test_expvar_exists(self):
        assert r.status_code == 200, r.status_code


-class MetricsIntegrationTest(Test):
+class MetricsIntegrationTest(ElasticTest):


simitt mentioned this pull request Aug 3, 2018

Pipeline: Add check if exists. Change cmd output. elastic/beats#7851

Merged

simitt force-pushed the 1032-default-pipelines branch 2 times, most recently from 8781f05 to 6ff61c3 Compare August 7, 2018 09:59

simitt force-pushed the 1032-default-pipelines branch from 6ff61c3 to d15ac3e Compare August 7, 2018 10:56

simitt mentioned this pull request Aug 7, 2018

Master update beats #1264

Merged

simitt force-pushed the 1032-default-pipelines branch from d15ac3e to 55006fe Compare August 7, 2018 12:22

jalvz reviewed Aug 7, 2018

View reviewed changes

simitt force-pushed the 1032-default-pipelines branch 2 times, most recently from d6a6221 to 83a7b27 Compare August 8, 2018 08:14

jalvz reviewed Aug 8, 2018

View reviewed changes

jalvz approved these changes Aug 8, 2018

View reviewed changes

simitt force-pushed the 1032-default-pipelines branch from febd6d6 to 9fae109 Compare August 10, 2018 13:40

simitt self-assigned this Aug 10, 2018

simitt added [zube]: Inbox and removed [zube]: Inbox labels Aug 10, 2018

simitt added the [zube]: In Review label Aug 10, 2018

jalvz reviewed Aug 13, 2018

View reviewed changes

simitt added 8 commits August 13, 2018 11:26

changes according to beats changes.

8fd2343

change test for failure msg for ci.

7f816a1

Update config option descriptions.

3731f1f

PR comments.

2a12f38

fix path problem.

eeab521

fix tests.

d83da29

Add comment to test setup.

424eb0e

simitt force-pushed the 1032-default-pipelines branch from 9fae109 to 424eb0e Compare August 13, 2018 09:29

simitt merged commit fc51041 into elastic:master Aug 13, 2018

zube bot added [zube]: Done and removed [zube]: In Review labels Aug 13, 2018

This was referenced Aug 14, 2018

Document Pipeline handling in APM #1282

Closed

Update copied docs and pipeline docs #1296

Merged

simitt deleted the 1032-default-pipelines branch August 21, 2018 12:43

graphaelli reviewed Aug 28, 2018

View reviewed changes

graphaelli mentioned this pull request Aug 30, 2018

Cherry-pick #7851 to 6.x: Pipeline: Add check if exists. Change cmd output. elastic/beats#8171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Pipeline registration and add default pipelines. #1258

Add Pipeline registration and add default pipelines. #1258

simitt commented Aug 3, 2018 •

edited by zube bot

Loading

simitt commented Aug 7, 2018

jalvz Aug 7, 2018

simitt Aug 7, 2018

jalvz Aug 7, 2018

jalvz Aug 7, 2018

simitt commented Aug 8, 2018

jalvz Aug 8, 2018

jalvz Aug 8, 2018

simitt Aug 8, 2018

jalvz Aug 8, 2018

simitt Aug 8, 2018

jalvz Aug 8, 2018

jalvz Aug 8, 2018

jalvz Aug 8, 2018

simitt Aug 9, 2018

jalvz Aug 9, 2018

jalvz Aug 8, 2018

simitt commented Aug 8, 2018

jalvz Aug 8, 2018

jalvz Aug 8, 2018

simitt Aug 8, 2018

jalvz Aug 8, 2018

jalvz left a comment

simitt commented Aug 9, 2018

simitt commented Aug 10, 2018

simitt commented Aug 10, 2018

simitt commented Aug 10, 2018

simitt commented Aug 13, 2018

jalvz Aug 13, 2018

jalvz commented Aug 13, 2018

graphaelli Aug 28, 2018

Add Pipeline registration and add default pipelines. #1258

Add Pipeline registration and add default pipelines. #1258

Conversation

simitt commented Aug 3, 2018 • edited by zube bot Loading

simitt commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simitt commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simitt commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalvz left a comment

Choose a reason for hiding this comment

simitt commented Aug 9, 2018

simitt commented Aug 10, 2018

simitt commented Aug 10, 2018

simitt commented Aug 10, 2018

simitt commented Aug 13, 2018

Choose a reason for hiding this comment

jalvz commented Aug 13, 2018

Choose a reason for hiding this comment

simitt commented Aug 3, 2018 •

edited by zube bot

Loading