Skip to content

Rename dataset to avoid ambiguous index pattern generation#4669

Merged
jalvz merged 3 commits intoelastic:masterfrom
jalvz:rename-dataset
Feb 2, 2021
Merged

Rename dataset to avoid ambiguous index pattern generation#4669
jalvz merged 3 commits intoelastic:masterfrom
jalvz:rename-dataset

Conversation

@jalvz
Copy link
Copy Markdown
Contributor

@jalvz jalvz commented Feb 1, 2021 •
edited
Loading

Motivation/summary

Solves the issue described in elastic/kibana#88307 (comment)

While we could maybe find a way out with priority ordering, that would introduce unnecessary complexity.

Happy to replace "user" with something more appropriate if there are better ideas.

How to test these changes

Wait for package 0.5.0 release

@jalvz jalvz requested a review from a team February 1, 2021 16:48
@ghost
Copy link
Copy Markdown

ghost commented Feb 1, 2021 •
edited by ghost
Loading

💚 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 #4669 updated

    • Start Time: 2021-02-02T09:50:30.166+0000

  • Duration: 31 min 6 sec

  • Commit: c2d62b8

Test stats 🧪

Test Results
Failed 0
Passed 4361
Skipped 123
Total 4484

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 25 sec . View more details on here
  • Description: ./.ci/scripts/sync.sh

axw
axw requested changes Feb 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Thanks, I'd forgotten about that one.

Please delete the checklist section in the description.

apmpackage/apm/0.1.0/data_stream/app_metrics/manifest.yml Outdated
title: APM application metrics
type: metrics
dataset: apm
dataset: apm.user
Copy link
Copy Markdown
Member

@axw axw Feb 2, 2021

Choose a reason for hiding this comment

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

Suggested change
dataset: apm.user
dataset: apm.app

The title of the stream is "app metrics" after all :)

Copy link
Copy Markdown
Contributor Author

@jalvz jalvz Feb 2, 2021

Choose a reason for hiding this comment

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

ha, I was 50/50 between "app" and "user"

@simitt
Copy link
Copy Markdown
Contributor

simitt commented Feb 2, 2021

This PR fixes the apm issue; we still need to find a more generic answer to the question around how to avoid such conflicts right?

@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 2, 2021

Codecov Report

Merging #4669 (8a08b0c) into master (d7336bd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4669   +/-   ##
=======================================
  Coverage   76.16%   76.16%           
=======================================
  Files         161      161           
  Lines        9892     9892           
=======================================
  Hits         7534     7534           
  Misses       2358     2358
Impacted Files Coverage Δ
model/metricset.go 95.08% <ø> (ø)

@jalvz jalvz mentioned this pull request Feb 2, 2021
Merged
1 task
@jalvz
Copy link
Copy Markdown
Contributor Author

jalvz commented Feb 2, 2021

This PR fixes the apm issue; we still need to find a more generic answer to the question around how to avoid such conflicts right?

There is no much about it, if one dataset name is prefix of another, it is going to be ambiguous. Maybe it can be validated in the package registry.

@jalvz jalvz merged commit 46ccd7a into elastic:master Feb 2, 2021
@jalvz jalvz mentioned this pull request Feb 2, 2021
Closed
axw pushed a commit to axw/apm-server that referenced this pull request Feb 19, 2021
@jalvz @axw
Rename dataset to avoid ambiguous index pattern generation (elastic#4669
4d11779 
)

# Conflicts:
#	apmpackage/apm/0.1.0/data_stream/app_metrics/manifest.yml
@axw axw mentioned this pull request Feb 19, 2021
Merged
axw pushed a commit to axw/apm-server that referenced this pull request Feb 19, 2021
@jalvz @axw
Rename dataset to avoid ambiguous index pattern generation (elastic#4669
904d0bc 
)

# Conflicts:
#	apmpackage/apm/0.1.0/data_stream/app_metrics/manifest.yml
@axw axw mentioned this pull request Feb 19, 2021
Merged
axw added a commit that referenced this pull request Feb 19, 2021
@axw @jalvz
Rename dataset to avoid ambiguous index pattern generation (#4669) (#…
69947f5 
…4838)

# Conflicts:
#	apmpackage/apm/0.1.0/data_stream/app_metrics/manifest.yml

Co-authored-by: Juan Álvarez <juan.alvarez@elastic.co>
axw added a commit that referenced this pull request Feb 19, 2021
@axw @jalvz
Rename dataset to avoid ambiguous index pattern generation (#4669) (#…
96f7d17 
…4837)

# Conflicts:
#	apmpackage/apm/0.1.0/data_stream/app_metrics/manifest.yml

Co-authored-by: Juan Álvarez <juan.alvarez@elastic.co>
@axw axw self-assigned this Feb 25, 2021
@axw
Copy link
Copy Markdown
Member

axw commented Feb 25, 2021

Confirmed with BC2 and epr-snapshot. All APM index templates have a prefix of apm. on the dataset:

image

Application metrics are being put into metrics-apm.app.<service>-* data streams:

image

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

Reviewers

@axw axw axw approved these changes

Assignees

@axw axw

Labels

test-plan test-plan-ok v7.12.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jalvz @simitt @codecov-io @axw