Skip to content

feat: Control log API through separate RUM flag #1467

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cwli24
Copy link
Contributor

@cwli24 cwli24 commented May 5, 2025

Logs captured by the newrelic.log method will now be configurable through settings and controlled separately from wrapped logs. This allows one to be disabled without affecting the other, as in the case that only newrelic.log is desired and not auto instrumented console.

Overview

Related Issue(s)

https://new-relic.atlassian.net/browse/NR-399179

Testing

Added

Copy link

github-actions bot commented May 5, 2025

Asset Size Report

Merging this pull request will result in the following asset size changes:

Agent Asset Previous Size New Size Diff
lite loader 24.8 kB / 9.41 kB (gzip) 24.8 kB / 9.41 kB (gzip) 0% / 0.01% (gzip)
lite async-chunk 52.82 kB / 17.51 kB (gzip) 52.84 kB / 17.52 kB (gzip) 0.04% / 0.05% (gzip)
pro loader 52.34 kB / 18.43 kB (gzip) 52.35 kB / 18.43 kB (gzip) 0.02% / 0.05% (gzip)
pro async-chunk 91.78 kB / 28.45 kB (gzip) 91.85 kB / 28.56 kB (gzip) 0.08% / 0.4% (gzip)
spa loader 61.1 kB / 21.13 kB (gzip) 61.11 kB / 21.13 kB (gzip) 0.02% / 0.01% (gzip)
spa async-chunk 116.4 kB / 35.42 kB (gzip) 116.48 kB / 35.54 kB (gzip) 0.07% / 0.34% (gzip)

Copy link

github-actions bot commented May 5, 2025

Static Badge

Last ran on May 05, 2025 14:47:26 CDT
Checking merge of (267d9c4) into main (c11b6de)

Copy link

codecov bot commented May 5, 2025

Codecov Report

Attention: Patch coverage is 79.16667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 87.86%. Comparing base (c11b6de) to head (267d9c4).
Report is 30 commits behind head on main.

Files with missing lines Patch % Lines
src/features/logging/aggregate/index.js 76.19% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1467      +/-   ##
==========================================
- Coverage   87.92%   87.86%   -0.06%     
==========================================
  Files         196      196              
  Lines        7744     7739       -5     
  Branches     1548     1549       +1     
==========================================
- Hits         6809     6800       -9     
- Misses        804      806       +2     
- Partials      131      133       +2     
Flag Coverage Δ
unit-tests 78.98% <79.16%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@@ -166,13 +168,18 @@ export class Aggregate extends AggregateBase {
this.events.clear()
this.events.clearSave()
}
this.updateLoggingMode(LOGGING_MODE.OFF)
this.loggingMode = {
Copy link
Member

Choose a reason for hiding this comment

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

Should this just call this.updateLocalLoggingMode to reduce the places that this.loggingMode is set?

@@ -14,5 +14,5 @@ export function setupLogAPI (agent) {
}

export function log (message, { customAttributes = {}, level = LOG_LEVELS.INFO } = {}, agentRef, targetEntityGuid, timestamp = now()) {
bufferLog(agentRef.ee, message, customAttributes, level, targetEntityGuid, timestamp)
bufferLog(agentRef.ee, message, customAttributes, level, false, targetEntityGuid, timestamp)
Copy link
Member

Choose a reason for hiding this comment

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

what is <api>.wrapLogger setting for this? Shouldnt that honor the API settings as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure. It wasn't clear if the goal was to configure just .log separate or .wrapLogger as well--or whether those two themselves should also be separate.

At the moment, only individual .log are controlled by the new flag, while wrapLogger is lumped with console

Copy link
Member

Choose a reason for hiding this comment

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

I believe the original "auto logging" documentation has solely described auto-implementation of the console APIs. I think it would make more sense to include wrapLogger in with the API sampling but I will defer to @ptang-nr who is leading the feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked with PM, wrapLogger should also follow API sampling 👍

@lmuraru-plenty
Copy link

Hi!

I would love to get my hands on this feature! Is there an approximate release timeline?

Currently, I find myself stuck having to allow my browser apps to ingest millions of unused logs monthly, just to get a few hundred relevant logs that I need to know about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants