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.

Sign up for GitHub

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

Jump to bottom

Prevent indexing of event.received field #11602

Closed
wants to merge 12 commits into from
Closed

Prevent indexing of event.received field #11602

wants to merge 12 commits into from

Conversation

endorama
Copy link
Member

@endorama endorama commented Sep 7, 2023 •
edited
Loading

Motivation/summary

We added support for event.received field in elastic/apm-data#85.

This field is useful to compute delay between event reception and ingestion, but is not currently used by APM UI.

To prevent ingesting unnecessary data, this PR adds a remove processor to ingest pipelines to prevent indexing event.received.

Data streams updated:

  • app_logs
  • app_metrics
  • error_logs
  • internal_metrics
  • rum_traces
  • traces

Note for reviewers: I'm not sure if this PR needs to be backported.

Checklist

How to test these changes

Related issues

Related to #11138

@mergify
Copy link
Contributor

mergify bot commented Sep 7, 2023

This pull request does not have a backport label. Could you fix it @endorama? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-7.17 is the label to automatically backport to the 7.17 branch.
  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Sep 7, 2023
carsonip
carsonip reviewed Sep 7, 2023
View reviewed changes
Copy link
Member

@carsonip carsonip left a comment

Choose a reason for hiding this comment

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

  1. Could you also check whether we need this for aggregated metrics data streams?
  2. Remove https://github.com/elastic/apm-server/blob/8.9/internal/beater/processors.go#L89 and its usage so that we can confirm the ingest pipeline works.
  3. System test to ensure the field is removed if possible

apmpackage/apm/data_stream/app_logs/elasticsearch/ingest_pipeline/default.yml Outdated
@@ -9,3 +9,8 @@ processors:
name: observer_ids
- pipeline:
name: ecs_version
- remove:
Copy link
Member

Choose a reason for hiding this comment

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

Even better, define a pipeline like the one in https://github.com/elastic/apm-server/blob/main/apmpackage/cmd/genpackage/pipelines.go#L155 so that it can be reused in multiple ingest pipelines.

@endorama
remove removeEventReceivedBatchProcessor
8ac04bd 
The same functionality is not handled in ingest pipelines
@endorama
add remove_event_received to genpackage
20051e9
@endorama
use shared pipeline
8e51d6b 
Replace previous implementation with the shared pipeline
@endorama endorama changed the title remove event.received from app_logs Prevent ingestion of event.received field Sep 7, 2023
@endorama endorama changed the title Prevent ingestion of event.received field Prevent indexing of event.received field Sep 7, 2023
@endorama endorama marked this pull request as ready for review September 7, 2023 15:32
@endorama endorama requested a review from a team as a code owner September 7, 2023 15:32
@endorama
Copy link
Member Author

endorama commented Sep 7, 2023

Could you also check whether we need this for aggregated metrics data streams?

Checked and I don't think we need to.

Remove 8.9/internal/beater/processors.go#L89 and its usage so that we can confirm the ingest pipeline works.

Done in 8ac04bd, thanks for pointing that out!

System test to ensure the field is removed if possible

I reviewed the system tests and this is already tested. When removing the removeEventReceivedBatchProcessor processor a series of tests fail:

  • TestJaeger
  • TestJaeger/batch_0
  • TestJaeger/batch_1
  • TestOTLPGRPCTraces
  • TestOTLPGRPCMetrics
  • TestOTLPGRPCLogs

The same tests pass when adding the ingest pipeline. Also is my understanding that as #11138 has not been completed APM events don't have event.received.

carsonip
carsonip reviewed Sep 7, 2023
View reviewed changes
apmpackage/apm/changelog.yml Outdated
@@ -12,6 +12,9 @@
- description: Remove unused `processor.event` field from logs data streams
type: enhancement
link: https://github.com/elastic/apm-server/pull/11494
- description: Remove `event.received` fields from logs, metrics and traces data streams
type: bugfix
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type: bugfix
type: enhancement

There's no bug in apmpackage (mapping and ingest pipeline) itself

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, followed up in cee297b

@endorama
Copy link
Member Author

endorama commented Sep 8, 2023

After internal discussion we prefer keeping the current approach instead of moving to an ingest pipeline. I'm closing this PR as is no longer relevant.

@endorama endorama closed this Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carsonip carsonip carsonip left review comments

Assignees
No one assigned
Labels
backport-skip Skip notification from the automated backport with mergify
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@endorama @carsonip