Skip to content

[sshshim] add sentry without using middleware - #366

Merged
savil merged 1 commit into
mainfrom
savil/cloud-ssh-sentry
Dec 14, 2022
Merged

savil merged 1 commit into
mainfrom
savil/cloud-ssh-sentry

Conversation

@savil

@savil savil commented Dec 13, 2022 •

Copy link
Copy Markdown
Collaborator

Summary

This PR is an alternative to both #361 and #363.

This PR's approach is lighterweight:

  1. refactor sentry code into its new telemetry package.
  2. directly call that in the sshshim command execution function.

The advantages it has are:

  1. Major: The implementation in [sshshim] use midcobra to execute command with sentry middleware #363 is rather gross because we need to introduce an interface equivalent of cobra.Command. This is really ugly, and not very maintainable. If new middleware uses other functions of cobra commands, then we'll have to add those to the interface and also handle them in the sshshim-command version.
  2. Minor: We keep the midcobra.telemetry middleware for the regular devbox command.

Disadvantages are:

  1. We may in the future need to incorporate new middleware into the sshshim command in a non-middleware manner.

How was it tested?

compiles.

Tested via:

  1. Look up sentryDSN in sentry dashboard > Settings > Project (devbox) > ClientKeys (DSN)
  2. Inserted that in internal/build/build.go
  3. Manually inserted errors in devbox.Open and invokeSSHOrSCPCommand. Ran devbox commands that triggered them.
  4. verified in sentry dashboard that the errors were logged.

savil commented Dec 13, 2022

Copy link
Copy Markdown
Collaborator Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@savil
savil requested review from LucilleH and gcurtis December 13, 2022 23:08
@savil
savil force-pushed the savil/cloud-ssh-sentry branch 2 times, most recently from 2b4ba7b to d6fa1f8 Compare December 13, 2022 23:21
@savil
savil marked this pull request as ready for review December 13, 2022 23:21
@savil
savil force-pushed the savil/cloud-ssh-sentry branch from d6fa1f8 to e6d06b2 Compare December 13, 2022 23:28

@LucilleH LucilleH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This works too. I don't feel strongly one way or another

@savil
savil merged commit 9ca32cb into main Dec 14, 2022
@savil
savil deleted the savil/cloud-ssh-sentry branch December 14, 2022 18:37
mikeland73 added a commit that referenced this pull request Sep 16, 2026
…de-extension (#2980)

## Summary

Follow-up to #2979. Clears the last **10 open Dependabot alerts** (8
medium, 2 low) — the repo now has zero open alerts. Each manifest was
re-locked using the toolchain its own `devbox.json` provides (via
`devbox shellenv`), then every package in every touched lockfile was
audited against [OSV](https://osv.dev): **283 unique packages, 0
advisories**.

| Location | Change | Alerts cleared |
| --- | --- | --- |
| `examples/development/python/poetry/poetry-demo` | pytest `7.4.4` →
`9.1.1`; python constraint `^3.8` → `^3.10` | #438, #368 |
| `examples/development/python/poetry/poetry-pyproject-subdir/service` |
pytest `7.4.4` → `9.1.1`; python constraint `^3.8` → `^3.10` | #439,
#367 |
| `examples/development/python/pipenv` | lock-only: pytest → `9.1.1`
(grpcio `1.84.0`, etc. moved along) | #366 |
| `examples/data_science/pytorch/basic-example` | lock-only: torch
`2.7.1` → `2.14.0`, setuptools `80.10.2` → `84.0.0` (transitive) | #453,
#440, #403, #402 |
| `vscode-extension` | lock-only: serialize-javascript `7.0.4` → `7.1.1`
| #383 |

### Notes

**pytest needed the Python floor raised.** The tmpdir fix
(GHSA-6w46-j5rx-g56g) only exists in 9.0.3+, and pytest 9 requires
Python ≥ 3.10, so the two poetry examples' `python = "^3.8"` constraint
had to move to `^3.10` for the fix to be reachable. Both examples'
`devbox.json` already install `python@latest`, so nothing changes for
anyone running them through devbox. `pytest = "^7.2.2"` → `"^9.0.3"` is
the only other manifest edit in the PR.

**torch moved to CUDA 13 wheels.** `torch = "^2.7.0"` already admitted
2.14.0, so `pyproject.toml` is untouched, but the Linux extras in the
lock shifted from `nvidia-*-cu12` to CUDA 13 packages (`cuda-toolkit
13.0.3`, `nvidia-cudnn-cu13`, etc.). The wheels bundle their own
runtime, so this matters only for driver version on Linux hosts (CUDA 13
needs a 580+ driver). The nix `cudatoolkit` pinned in that example's
`devbox.lock` is 11.7 from an old nixpkgs and was already mismatched
with the previous cu12 wheels — I left it alone as it's unrelated to the
advisories.

**Pre-existing, not fixed here:** the pytorch example's `poetry install`
fails on main because `pyproject.toml` declares `packages = [{include =
"devbox_cuda_dev"}]` and that directory doesn't exist. `poetry install
--no-root` works; I verified torch 2.14.0 imports and runs on CPU that
way.

**vscode-extension:** the `resolutions` entry already allowed `^7.0.0`,
so only `yarn.lock` moved. While there, `yarn audit` flagged `ajv
6.12.6` (GHSA-2g4f-4pwh-qvx6) and `diff 5.2.0` (GHSA-73rr-hh4g-fpgx);
both patches fall inside existing ranges so they were refreshed too.
`yarn audit --level low` is now clean.

## How was it tested?

- `devbox run test` (poetry-demo) and `devbox run run_test`
(poetry-pyproject-subdir): 1 passed each.
- `devbox run run_test` (pipenv): runs `main.py` successfully.
- pytorch example: `poetry install --no-root` + import/matmul smoke test
→ `torch 2.14.0, numpy 1.26.4, setuptools 84.0.0`.
- vscode-extension: `yarn install --frozen-lockfile`, `yarn compile`,
`yarn lint`, `yarn audit --level low` → 0 vulnerabilities.
- OSV batch query over every package in all five lockfiles → 0
advisories.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants