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

Remove service datasets #4491

Merged
merged 6 commits into from
Dec 8, 2020
Merged

Conversation

jalvz
Copy link
Contributor

@jalvz jalvz commented Dec 4, 2020

Motivation/summary

We need to remove service names from index names, as they need to manage exactly what Fleet expects for data streams to be created.

Checklist

I have considered changes for:

How to test these changes

Related issues

@jalvz jalvz force-pushed the remove-service-datasets branch from 7cc5d7b to 02c1cb5 Compare December 4, 2020 12:09
@codecov-io
Copy link

codecov-io commented Dec 4, 2020

Codecov Report

Merging #4491 (8e4d215) into master (6062235) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4491      +/-   ##
==========================================
+ Coverage   75.91%   75.96%   +0.05%     
==========================================
  Files         161      161              
  Lines        9790     9783       -7     
==========================================
  Hits         7432     7432              
+ Misses       2358     2351       -7     
Impacted Files Coverage Δ
datastreams/servicename.go 100.00% <ø> (ø)
model/error.go 98.58% <100.00%> (-0.01%) ⬇️
model/metricset.go 94.73% <100.00%> (-0.10%) ⬇️
model/profile.go 100.00% <100.00%> (ø)
model/span.go 93.10% <100.00%> (-0.08%) ⬇️
model/transaction.go 96.15% <100.00%> (-0.08%) ⬇️
...ack/apm-server/aggregation/txmetrics/aggregator.go 93.36% <0.00%> (ø)
processor/otel/consumer.go 93.95% <0.00%> (+0.44%) ⬆️
kibana/connecting_client.go 70.17% <0.00%> (+8.77%) ⬆️

@apmmachine
Copy link
Contributor

apmmachine commented Dec 4, 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: Branch indexing

  • Start Time: 2020-12-08T02:50:09.963+0000

  • Duration: 43 min 44 sec

Test stats 🧪

Test Results
Failed 0
Passed 4621
Skipped 139
Total 4760

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

@jalvz jalvz force-pushed the remove-service-datasets branch from 02c1cb5 to 6f59fee Compare December 4, 2020 12:13
@jalvz jalvz force-pushed the remove-service-datasets branch from 6f59fee to eaa25cf Compare December 4, 2020 12:13
@jalvz jalvz force-pushed the remove-service-datasets branch from eaa25cf to 122fd25 Compare December 4, 2020 12:16
@jalvz jalvz added the bug label Dec 4, 2020
@axw
Copy link
Member

axw commented Dec 7, 2020

The main changes looks fine, but what's up with the pipeline removal? If it's not related to this change, can you please separate that into another PR?

@jalvz
Copy link
Contributor Author

jalvz commented Dec 7, 2020

The pipeline removal is a diff artifact, and looks "re-added" here: apmpackage/apm/0.1.0/data_stream/profile_metrics/elasticsearch/ingest_pipeline/default.json. But it is the same with the name fixed.

The reason for the pipeline changes is that the profiles one had the wrong name (s/profiles-apm-0.1.0/metrics-apm.profiling-0.1.0) which I blamed on poor data stream naming, so I took the chance to rename the data streams as well.

I made package and code changes into separated commits, but happy to move one out to another PR if you prefer.

Copy link
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.

The pipeline removal is a diff artifact, and looks "re-added" here: apmpackage/apm/0.1.0/data_stream/profile_metrics/elasticsearch/ingest_pipeline/default.json. But it is the same with the name fixed.

The reason for the pipeline changes is that the profiles one had the wrong name (s/profiles-apm-0.1.0/metrics-apm.profiling-0.1.0) which I blamed on poor data stream naming, so I took the chance to rename the data streams as well.

I made package and code changes into separated commits, but happy to move one out to another PR if you prefer.

I see. Since you're reusing the constant now, it makes sense to have them in the same PR. Thanks for the explanation.

@jalvz jalvz merged commit abb6efe into elastic:master Dec 8, 2020
@jalvz jalvz mentioned this pull request Dec 8, 2020
15 tasks
jalvz added a commit to jalvz/apm-server that referenced this pull request Dec 15, 2020
Also rename data streams and fix the profiles pipelines referenced names
jalvz added a commit that referenced this pull request Dec 15, 2020
… (#4537)

* Fix pipeline name refs (#4489)

Update pipeline names referenced in default.json files to match the one generated by Fleet.

* Remove service datasets (#4491)

Also rename data streams and fix the profiles pipelines referenced names
@simitt
Copy link
Contributor

simitt commented Dec 23, 2020

Not sure this needs any manual testing/how to test this manually. I'll leave it there fore now @jalvz can you please leave a comment on what to test here if anything should be done, otherwise add the test_plan_skip label - thank you.

@jalvz
Copy link
Contributor Author

jalvz commented Jan 4, 2021

Probably the test of this is covered already by some other issue. Basically, data streams should be created when data is ingested. It was not the case before this.

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.

5 participants