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

aggregation/txmetrics: drop user-agent parsing #4439

Merged
merged 1 commit into from
Nov 18, 2020

Conversation

axw
Copy link
Member

@axw axw commented Nov 18, 2020

Motivation/summary

Remove user-agent from transaction metrics, and all related config and processing. RUM-specific charts have been removed from the APM app, and these metrics are not currently required by the User Experience app. Drop the TODOs about adding country name while we're at it.

Checklist

I have considered changes for:
- [ ] documentation
- [ ] logging (add log lines, choose appropriate log selector, etc.)
- [ ] metrics and monitoring (create issue for Kibana team to add metrics to visualizations, e.g. Kibana#44001)

How to test these changes

  1. Run server with transaction metrics enabled.
  2. Send some RUM page-loads: same page with different browsers.
  3. Observe that transaction metrics are aggregated, but user-agent is not included and the page-loads from multiple browsers are grouped together.

Related issues

#3613 (comment)
elastic/kibana#77458

RUM-specific charts have been removed from the
APM app, and these metrics are not currently
required by the User Experience app.
@codecov-io
Copy link

Codecov Report

Merging #4439 (5ae0137) into master (b3759fc) will increase coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4439      +/-   ##
==========================================
+ Coverage   76.14%   76.21%   +0.07%     
==========================================
  Files         157      156       -1     
  Lines        9724     9694      -30     
==========================================
- Hits         7404     7388      -16     
+ Misses       2320     2306      -14     
Impacted Files Coverage Δ
beater/config/aggregation.go 100.00% <ø> (ø)
...ack/apm-server/aggregation/txmetrics/aggregator.go 93.36% <ø> (+1.27%) ⬆️
x-pack/apm-server/main.go 0.00% <ø> (ø)

@apmmachine
Copy link
Contributor

💚 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 #4439 opened]

  • Start Time: 2020-11-18T03:48:50.344+0000

  • Duration: 54 min 20 sec

Test stats 🧪

Test Results
Failed 0
Passed 4751
Skipped 145
Total 4896

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 20 sec . View more details on here
  • Description: ./script/jenkins/sync.sh

@axw axw requested a review from a team November 18, 2020 05:54
@axw axw merged commit 33e1a92 into elastic:master Nov 18, 2020
@axw axw deleted the txmetrics-remove-rum branch November 18, 2020 08:51
axw added a commit to axw/apm-server that referenced this pull request Dec 10, 2020
RUM-specific charts have been removed from the
APM app, and these metrics are not currently
required by the User Experience app.
axw added a commit that referenced this pull request Dec 14, 2020
RUM-specific charts have been removed from the
APM app, and these metrics are not currently
required by the User Experience app.
@simitt
Copy link
Contributor

simitt commented Dec 23, 2020

Tested with BC1 - sent transactions with equal and different user agent request headers and observed no difference in grouping.

@simitt simitt self-assigned this Dec 23, 2020
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