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(dd): add traces to gitfilesysteM #1240

Merged
merged 12 commits into from
Mar 27, 2024

Conversation

kishore03109
Copy link
Contributor

@kishore03109 kishore03109 commented Mar 25, 2024

Problem

we are having divergence, still not enough information to determine why. adding traces to the major gitfile system operations to give a better idea of what is happening.

Notes to all dev

Once this PR is merged all devs will need to call npm install to:

  1. get the updated version of dd-trace
  2. ensure that patch-package runs to patch dd-trace and make it compatible with neverthrow

Deploy Notes

This PR records a lot of new spans into traces. Basically ALL GitFileSystem operations are now instrumented.
During release, close attention need to be given to system load to ensure the new instrumentation is not adding too high a CPU cost to the system.

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @kishore03109 and the rest of your teammates on Graphite Graphite

@kishore03109 kishore03109 force-pushed the 03-25-feat_dd_add_traces_to_gitfilesysteM branch 7 times, most recently from f347e5d to 82171bb Compare March 26, 2024 02:24
@timotheeg timotheeg force-pushed the 03-25-feat_dd_add_traces_to_gitfilesysteM branch 3 times, most recently from e64fdcb to bf3e618 Compare March 26, 2024 09:17
@timotheeg timotheeg marked this pull request as ready for review March 27, 2024 01:43
@timotheeg timotheeg requested a review from a team March 27, 2024 01:43
@timotheeg
Copy link
Contributor

timotheeg commented Mar 27, 2024

  • Added tests to document the compatibility behaviour of dd-trace and neverthrow
  • Upgraded dd-trace to v5 and patched for neverthrow compatibility again (we should check if our docker uses the latest dd-agent too...)

@timotheeg
Copy link
Contributor

Test on staging with dd-trace V5, sample trace here for reference

Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

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

LGTM - tested on staging

Warning on potential cost:

  • runtime cpu cost since we now instrument ALL the git actions
  • potential monetary cost if we index a lot more spans

Must monitor closely at release

@seaerchin
Copy link
Contributor

@timotheeg @kishore03109 we seem to have git patches here? not sure if this was intended

@timotheeg
Copy link
Contributor

we seem to have git patches here? not sure if this was intended

It's intended. We have to patch dd-trace to make it compatible with neverthrow. This is done with using patch-package

@timotheeg
Copy link
Contributor

timotheeg commented Mar 27, 2024

  • Removed one of the patch for [email protected], since we're locking the version to 5.9 now
  • Added blacklist of methods to instrument, to reduce span noise just a little

@kishore03109 kishore03109 force-pushed the 03-25-feat_dd_add_traces_to_gitfilesysteM branch from 89be271 to 69fa04b Compare March 27, 2024 23:24
@kishore03109 kishore03109 merged commit af8649f into develop Mar 27, 2024
10 checks passed
@mergify mergify bot deleted the 03-25-feat_dd_add_traces_to_gitfilesysteM branch March 27, 2024 23:26
This was referenced Mar 28, 2024
kishore03109 added a commit that referenced this pull request Mar 28, 2024
* chore(package): use npm (#1237)

* fix: upgrade dotenv from 16.4.1 to 16.4.5 (#1233)

Snyk has created this PR to upgrade dotenv from 16.4.1 to 16.4.5.

See this package in npm:
https://www.npmjs.com/package/dotenv

See this project in Snyk:
https://app.snyk.io/org/isomer/project/676b9e26-cebf-4964-b7b3-d9843e3339ff?utm_source=github&utm_medium=referral&page=upgrade-pr

Co-authored-by: snyk-bot <[email protected]>

* build(deps): bump express from 4.17.3 to 4.19.2 (#1242)

Bumps [express](https://github.com/expressjs/express) from 4.17.3 to 4.19.2.
- [Release notes](https://github.com/expressjs/express/releases)
- [Changelog](https://github.com/expressjs/express/blob/master/History.md)
- [Commits](expressjs/express@4.17.3...4.19.2)

---
updated-dependencies:
- dependency-name: express
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: package.json & package-lock.json to reduce vulnerabilities (#1241)

The following vulnerabilities are fixed with an upgrade:
- https://snyk.io/vuln/SNYK-JS-EXPRESS-6474509

Co-authored-by: snyk-bot <[email protected]>

* fix: upgrade @growthbook/growthbook from 0.27.0 to 0.34.0 (#1232)

Snyk has created this PR to upgrade @growthbook/growthbook from 0.27.0 to 0.34.0.

See this package in npm:
https://www.npmjs.com/package/@growthbook/growthbook

See this project in Snyk:
https://app.snyk.io/org/isomer/project/676b9e26-cebf-4964-b7b3-d9843e3339ff?utm_source=github&utm_medium=referral&page=upgrade-pr

Co-authored-by: snyk-bot <[email protected]>

* fix: upgrade @aws-sdk/client-dynamodb from 3.501.0 to 3.521.0 (#1229)

Snyk has created this PR to upgrade @aws-sdk/client-dynamodb from 3.501.0 to 3.521.0.

See this package in npm:
https://www.npmjs.com/package/@aws-sdk/client-dynamodb

See this project in Snyk:
https://app.snyk.io/org/isomer/project/676b9e26-cebf-4964-b7b3-d9843e3339ff?utm_source=github&utm_medium=referral&page=upgrade-pr

Co-authored-by: snyk-bot <[email protected]>

* fix: upgrade @aws-sdk/client-amplify from 3.501.0 to 3.521.0 (#1230)

Snyk has created this PR to upgrade @aws-sdk/client-amplify from 3.501.0 to 3.521.0.

See this package in npm:
https://www.npmjs.com/package/@aws-sdk/client-amplify

See this project in Snyk:
https://app.snyk.io/org/isomer/project/676b9e26-cebf-4964-b7b3-d9843e3339ff?utm_source=github&utm_medium=referral&page=upgrade-pr

Co-authored-by: snyk-bot <[email protected]>

* fix: upgrade @aws-sdk/client-cloudwatch-logs from 3.501.0 to 3.521.0 (#1231)

Snyk has created this PR to upgrade @aws-sdk/client-cloudwatch-logs from 3.501.0 to 3.521.0.

See this package in npm:
https://www.npmjs.com/package/@aws-sdk/client-cloudwatch-logs

See this project in Snyk:
https://app.snyk.io/org/isomer/project/676b9e26-cebf-4964-b7b3-d9843e3339ff?utm_source=github&utm_medium=referral&page=upgrade-pr

Co-authored-by: snyk-bot <[email protected]>

* fix(dockerfile): add dig to image (#1244)

* feat(dd): add traces to gitfilesysteM (#1240)

* feat(dd): adding in metrics to gitfilesystem

* feat: instrument with wrap-tracers

* feat: patch dd-trace to understand neverthrow

* fix: missing tracer import

* fix: use the clear tracer.wrap() method

* fix(dd): fix patch type

* chore: add tests for tracer compatibility with neverthrow

* chore: upgrade dd-trace to v5 and patch for neverthrow

* chore: remove unecessary change from patch

* feat: implement a blacklist for methods that should not be instrumented

* chore: remove obsolete patch

* chore: lock dd-trace to 5.9.0 strictly so patch is always applied

---------

Co-authored-by: Timothee Groleau <[email protected]>

* fix: reduce log size to just last commit (#1243)

* fix: reduce log size to just last commit

* feat: defaults maxCount in getGitLog to 1

* chore(GitFileSystemService): add validation tests for getGitLog

* chore(dep): add flag (#1247)

* chore: bump version to v0.73.0

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: seaerchin <[email protected]>
Co-authored-by: Isomer Admin <[email protected]>
Co-authored-by: snyk-bot <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Timothee Groleau <[email protected]>
Co-authored-by: Timothee Groleau <[email protected]>
@timotheeg timotheeg mentioned this pull request Apr 2, 2024
9 tasks
This was referenced Jun 27, 2024
@dcshzj dcshzj mentioned this pull request Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants