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

Duplicate common fields #4446

Merged
merged 7 commits into from
Nov 20, 2020
Merged

Duplicate common fields #4446

merged 7 commits into from
Nov 20, 2020

Conversation

jalvz
Copy link
Contributor

@jalvz jalvz commented Nov 19, 2020

Motivation/summary

This copies all the fields in fields.common.yml to the _meta/fields.yml files of each type.
This is needed for the APM integration, particularly for docs as they will be generated out of thefields.yml files of each type.
This results in duplicated fields.

Checklist

I have considered changes for:

How to test these changes

Run setup template and check that is unchanged.

Related issues

@jalvz jalvz marked this pull request as draft November 19, 2020 11:04
@apmmachine
Copy link
Contributor

apmmachine commented Nov 19, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #4446 updated]

  • Start Time: 2020-11-20T08:05:25.354+0000

  • Duration: 46 min 24 sec

Test stats 🧪

Test Results
Failed 0
Passed 4752
Skipped 143
Total 4895

Steps errors 3

Expand to view the steps failures

Compress

  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=coverage-files.tgz -czf coverage-files.tgz coverage

Compress

  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=system-tests-linux-files.tgz -czf system-tests-linux-files.tgz system-tests

Test Sync

  • Took 3 min 9 sec . View more details on here
  • Description: ./script/jenkins/sync.sh

@jalvz jalvz marked this pull request as ready for review November 19, 2020 12:29
@jalvz jalvz requested a review from a team November 19, 2020 12:29
- name: data_stream.namespace
type: keyword
description: User-defined data stream namespace.
example: production
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big thing, but as soon as we add the data_stream fields here they will be in the template and show up at the docs. Do we really want this at this point? Why do we need the fields here already?

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 comes from merging @axw changes, so I'll summon him to answer this questions :)

(I personally think it is ok thou, as that is always the case for any new fields added - they show up in the master docs before the actual release)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, we already have the fields in the templates. I got confused with having the fields in the dedicated base-field.yml in the other PR.

Will take this to an offline discussion about why we introduced the fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got confused with having the fields in the dedicated base-field.yml in the other PR.

Totally reasonable!

Copy link
Member

Choose a reason for hiding this comment

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

I probably should not have added those fields as they're really only meaningful when running under Agent. We can remove them in a followup, as there will probably be some fallout in tests.

@jalvz jalvz merged commit d22b1a6 into elastic:master Nov 20, 2020
@jalvz
Copy link
Contributor Author

jalvz commented Nov 20, 2020

cc @bmorelli25 field reference docs will look different because of this, there is no longer a "common fields" section, instead all fields are duplicated for each type (transaction, span, etc) - Like beats does. Let me know if you spot something odd!

@jalvz jalvz mentioned this pull request Nov 23, 2020
15 tasks
jalvz added a commit to jalvz/apm-server that referenced this pull request Dec 10, 2020
# Conflicts:
#	include/fields.go
jalvz added a commit that referenced this pull request Dec 10, 2020
# Conflicts:
#	include/fields.go
@axw
Copy link
Member

axw commented Dec 22, 2020

I ran apm-server export template for 7.10 and 7.11 BC1, and compared them. The differences were:

  • various ECS field additions
  • various ECS fields changed from keyword to wildcard
  • removal of the accidental "tracing" ECS fields
  • labels_string, labels_boolean, and labels_* dynamic templates are repeated. This seems undesirable. I'll open an issue for that, otherwise looks good.

@axw
Copy link
Member

axw commented Dec 22, 2020

Opened #4576

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants