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

feat: add error.transaction.name #2539

Merged
merged 5 commits into from Jan 24, 2022
Merged

Conversation

trentm
Copy link
Member

@trentm trentm commented Jan 19, 2022

Add transaction.name to errors captured in the context of a
transaction. This allows APM UI to have fields used for transaction
grouping, which allows correlating error groups to transaction groups.

Closes: #2456

Checklist

Add `transaction.name` to errors captured in the context of a
transaction. This allows APM UI to have fields used for transaction
grouping, which allows correlating error groups to transaction groups.

Closes: #2456
@trentm trentm self-assigned this Jan 19, 2022
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Jan 19, 2022
@elastic-apm-tech elastic-apm-tech added this to In Progress in APM-Agents (OLD) Jan 19, 2022
@trentm
Copy link
Member Author

trentm commented Jan 19, 2022

  • Sanity check that we don't need to only add error.transaction.name for recent APM Server versions. This was added in APM Server 8.0.

Correct. We do not. Per https://json-schema.org/understanding-json-schema/reference/object.html#additional-properties "By default any additional properties are allowed."

@apmmachine
Copy link
Collaborator

apmmachine commented Jan 19, 2022

💚 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 preview

Expand to view the summary

Build stats

  • Start Time: 2022-01-24T15:54:57.412+0000

  • Duration: 20 min 27 sec

  • Commit: 3ac23f0

Test stats 🧪

Test Results
Failed 0
Passed 243005
Skipped 0
Total 243005

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules

  • run benchmark tests : Run the benchmark test only.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@trentm
Copy link
Member Author

trentm commented Jan 19, 2022

Spurious failure in GH Action "Test / test-vers (17)" test run:

# system.memory.actual.free
ok 74 is present
ok 75 is a number
ok 76 is finite (was: 2799632384)
not ok 77 is larger than os.freemem() (value: 2799632384, free: 2799632384)
  ---
    operator: ok
    expected: true
    actual:   false
    at: Object.system.memory.actual.free (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/metrics/index.test.js:83:13)
    stack: |-
      Error: is larger than os.freemem() (value: 2799632384,
                free: 2799632384)
          at Test.assert [as _assert] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:314:54)
          at Test.bound [as _assert] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:99:32)
          at Test.assert (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:433:10)
          at Test.bound [as ok] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:99:32)
          at Object.system.memory.actual.free (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/metrics/index.test.js:83:13)
          at Object.sendMetricSet (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/metrics/index.test.js:142:20)
          at /home/runner/work/apm-agent-nodejs/apm-agent-nodejs/lib/metrics/reporter.js:57:34
          at /home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/after-all-results/index.js:20:25
          at processTicksAndRejections (node:internal/process/task_queues:78:11)
  ...

I wonder if /proc/mem's "MemFree" can be the same (or close, given we are sampling at different times) to the "MemAvailable" -- though it is typically smaller. If so, then this is a flaky test and the test condition should be changed.

Update: ^^ handled in #2540

@trentm trentm requested a review from astorm January 20, 2022 17:41
@trentm trentm marked this pull request as ready for review January 20, 2022 17:41
Copy link
Contributor

@astorm astorm left a comment

Choose a reason for hiding this comment

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

👍 Does the thing, no obvious negative consequences. Approving.

@trentm trentm merged commit 9811e58 into main Jan 24, 2022
APM-Agents (OLD) automation moved this from In Progress to Done Jan 24, 2022
@trentm trentm deleted the trentm/issue2456-trans-name-to-errors branch January 24, 2022 18:29
astorm pushed a commit that referenced this pull request Feb 4, 2022
Add `transaction.name` to errors captured in the context of a
transaction. This allows APM UI to have fields used for transaction
grouping, which allows correlating error groups to transaction groups.

Closes: #2456
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
Development

Successfully merging this pull request may close these issues.

[META 531] Add transaction.name to errors
3 participants