search

evanderkoogh / otel-cf-workers

An OpenTelemetry compatible library for instrumenting and exporting traces for Cloudflare Workers
BSD 3-Clause "New" or "Revised" License
210 stars 40 forks source link

test scaffolding and DO storage instrumentation tests #35

Open rtbenfield opened 11 months ago

rtbenfield commented 11 months ago

This PR introduces Vitest with initial unit tests covering Durable Object storage instrumentation. These tests are focused on verifying the resulting spans match the expected behavior.

In writing these tests, I found that if Durable Object storage calls fail then the spans are not captured. The related tests are skipped until implementing a fix in a follow-up PR.

rtbenfield commented 11 months ago

It seems like the GitHub Actions pipeline isn't running since this is coming from a fork. I thought pull_request_target should enable that, but maybe it has to be on main first 🤔 @evanderkoogh do you know if there is a different trigger needed here?

evanderkoogh commented 11 months ago

I have never used GitHub actions to be fair, but I do know that you really don't want to be automatically building/running code from forks, because people will just mine the latest shitty crypto coin from your account.

But I have added you to the repository, so you should be able to test this for yourself now :)

DaniFoldi commented 11 months ago

Workflows triggered by pull_request_target use the workflow in the base branch, which doesn't contain the workflow file yet. I agree with @evanderkoogh that running code from forks introduces a security risk, so best way is to let a maintainer write the action in an internal PR.

(Docs here: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks#about-workflow-runs-from-public-forks)

evanderkoogh commented 11 months ago

Hey @rtbenfield, I had to do some repository dark magic to remove a file that shouldn't have been in the repository. It would probably be easiest to redo this PR?

rtbenfield commented 11 months ago

Hey @rtbenfield, I had to do some repository dark magic to remove a file that shouldn't have been in the repository. It would probably be easiest to redo this PR?

Yeah no problem! I have the changes in my own branch so I can rebase and raise a new PR.

evanderkoogh commented 9 months ago

Hey @rtbenfield, do I need to keep this PR around still? Would love to get this merged and get some tests in here :)