Skip to content

chore: make dns and fs optional instrumentation #281

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
Merged
merged 1 commit into from
Apr 4, 2025
Merged

chore: make dns and fs optional instrumentation #281

merged 1 commit into from
Apr 4, 2025

Conversation

saidsef
Copy link
Owner

@saidsef saidsef commented Apr 4, 2025

  • chore: make dns and fs optional instrumentation

@saidsef saidsef self-assigned this Apr 4, 2025
github-actions[bot]
github-actions bot previously approved these changes Apr 4, 2025
View reviewed changes
@saidsef saidsef added documentation Improvements or additions to documentation enhancement New feature or request javascript Pull requests that update Javascript code preview Preview labels Apr 4, 2025
@saidsef
Copy link
Owner Author

saidsef commented Apr 4, 2025

/code pr

@saidsef saidsef requested a review from Copilot April 4, 2025 08:52
saidsef

This comment was marked as outdated.

Sign in to view

Copilot

This comment was marked as outdated.

Sign in to view

@saidsef saidsef force-pushed the optional-dns-fs-instrumentation branch from b22d0b0 to 6ed461d Compare April 4, 2025 08:54
github-actions[bot]
github-actions bot previously approved these changes Apr 4, 2025
View reviewed changes
@saidsef saidsef force-pushed the optional-dns-fs-instrumentation branch from 6ed461d to c865083 Compare April 4, 2025 08:55
@saidsef-pr
Copy link

saidsef-pr bot commented Apr 4, 2025

Application tracing-node-c865083-281-pr-reviews is in state Running.
See more here: https://argocd.saidsef.co.uk/applications/tracing-node-c865083-281-pr-reviews?operation=true

@saidsef-pr
Copy link

saidsef-pr bot commented Apr 4, 2025

Application tracing-node-c865083-281-pr-reviews has been created.

@saidsef-pr
Copy link

saidsef-pr bot commented Apr 4, 2025

Application tracing-node-c865083-281-pr-reviews is in state Succeeded.
See more here: https://argocd.saidsef.co.uk/applications/tracing-node-c865083-281-pr-reviews?operation=true

@saidsef
Copy link
Owner Author

saidsef commented Apr 4, 2025

/code pr

saidsef
saidsef commented Apr 4, 2025
View reviewed changes
Copy link
Owner Author

@saidsef saidsef left a comment

Choose a reason for hiding this comment

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

🤖
The PR looks good, making FS and DNS instrumentation optional with new boolean flags that default to false. This improves the library by giving users more control over which instrumentations are enabled, which can help reduce overhead when these features aren't needed.

A few observations:

  1. The code changes in libs/index.mjs are well implemented with:

    • Clear parameter documentation in JSDoc comments
    • Conditional instrumentation loading with explanatory comments
    • Reorganized instrumentation list to make the optional components clearer

  2. The README updates appropriately document the new parameters.

  3. The version bump from 3.4.5 to 3.4.6 is appropriate for this minor feature enhancement.

This change provides a good balance between functionality and performance, allowing users to opt-in to potentially higher-overhead instrumentations only when needed.

> claude-3-7-sonnet-latest [pr | 1]

@saidsef
Copy link
Owner Author

saidsef commented Apr 4, 2025

/code pr

saidsef
saidsef commented Apr 4, 2025
View reviewed changes
Copy link
Owner Author

@saidsef saidsef left a comment

Choose a reason for hiding this comment

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

🤖

Code Review Summary

Changes Overview

  • Made FS and DNS instrumentation optional with new boolean config options
  • Updated version from 3.4.5 to 3.4.6
  • Updated git revision reference in deployment file

Per-File Analysis

README.md

  • Added documentation for two new configuration options: enableFsInstrumentation and enableDnsInstrumentation, both defaulting to false.

deployment/base/job.yml

  • Updated the git revision reference from "dependabot-npm_and_yarn-opentelemetry-instrumentation-aws-sdk-0.50.0" to "optional-dns-fs-instrumentation".

libs/index.mjs

  • Added optional parameters for enabling FS and DNS instrumentation with clear documentation.
  • Refactored instrumentation registration to conditionally include FS and DNS based on the new parameters.

package.json and package-lock.json

  • Incremented version from 3.4.5 to 3.4.6.
  • Updated caniuse-lite dependency in package-lock.json from 1.0.30001709 to 1.0.30001710.

Issues and Improvements

  • Improvement: The default values for new instrumentation options are explained in comments, but consider adding examples in the README.md for enabling these instrumentations.

  • Style: In libs/index.mjs, move all instrumentations into a single array declaration rather than starting with a base array and then conditionally pushing to it.

  • Performance: Good improvement by making FS and DNS instrumentation optional, as these can create substantial overhead when not needed.

> claude-3-7-sonnet-latest [pr | 0]

@saidsef-pr
Copy link

saidsef-pr bot commented Apr 4, 2025

Application tracing-node-c865083-281-pr-reviews is now running new version of deployments manifests.
See more here: https://argocd.saidsef.co.uk/applications/tracing-node-c865083-281-pr-reviews?operation=true

@saidsef-pr
Copy link

saidsef-pr bot commented Apr 4, 2025

Application tracing-node-c865083-281-pr-reviews is in state Running.
See more here: https://argocd.saidsef.co.uk/applications/tracing-node-c865083-281-pr-reviews?operation=true

@saidsef-pr
Copy link

saidsef-pr bot commented Apr 4, 2025

Application tracing-node-c865083-281-pr-reviews is in state Succeeded.
See more here: https://argocd.saidsef.co.uk/applications/tracing-node-c865083-281-pr-reviews?operation=true

@saidsef-pr
Copy link

saidsef-pr bot commented Apr 4, 2025

Application tracing-node-c865083-281-pr-reviews is now running new version of deployments manifests.
See more here: https://argocd.saidsef.co.uk/applications/tracing-node-c865083-281-pr-reviews?operation=true

@saidsef-pr
Copy link

saidsef-pr bot commented Apr 4, 2025

Application tracing-node-c865083-281-pr-reviews is in state Running.
See more here: https://argocd.saidsef.co.uk/applications/tracing-node-c865083-281-pr-reviews?operation=true

@saidsef-pr
Copy link

saidsef-pr bot commented Apr 4, 2025

Application tracing-node-c865083-281-pr-reviews is in state Succeeded.
See more here: https://argocd.saidsef.co.uk/applications/tracing-node-c865083-281-pr-reviews?operation=true

@saidsef saidsef merged commit ca9d24c into main Apr 4, 2025
8 checks passed
@saidsef saidsef deleted the optional-dns-fs-instrumentation branch April 4, 2025 09:07
@saidsef-pr
Copy link

saidsef-pr bot commented Apr 4, 2025

Application tracing-node-c865083-281-pr-reviews has been deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@github-actions github-actions[bot] github-actions[bot] approved these changes

Assignees

@saidsef saidsef

Labels
documentation Improvements or additions to documentation enhancement New feature or request javascript Pull requests that update Javascript code preview Preview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@saidsef