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

Adding JS bindings to @axiomhq/js for /v2/monitors endpoint support #263

Open
ThetaBird opened this issue Jan 27, 2025 · 4 comments
Open

Comments

@ThetaBird
Copy link

I'm seeing that @axiomhq/js doesn't yet have JS bindings for Axiom's /v2/monitors API endpoints. I would like to bring the library one step closer to full parity by creating a PR with the functionality if there isn't any internal work in progress to get this added in the near future.

My goals for the PR would consist of the following:

  • Create new file packages/js/src/monitors.ts with JS bindings using axiom-go Monitor struct for reference (thanks to @thecraftman for the pointer!) and following the design patterns used in neighboring services
  • Add tests in new file packages/js/src/test/unit/monitors.test.ts for testing CRUD operations
  • Add examples in new files examples/js/src for list-monitors, get-monitor, create-monitor, update-monitor, and delete-monitor

The current plan is to do all this in one PR, if I get verbal approval to continue with this. Open to splitting things out into multiple PRs if requested.

I think this would be a good starting point to get familiarized with the contributing process - adding bindings should be relatively straight forward and I might find the time to port other services as well.

Please let me know if this is a good idea and/or whether I am missing anything in the outline!

@dasfmi
Copy link
Collaborator

dasfmi commented Jan 29, 2025

hi @ThetaBird , that sounds great, your contribution is very welcomed, me or @gabrielelpidio can help with reviewing & merging it. Having the changes you mentioned in one PR should work out the best, go for it.

@ThetaBird
Copy link
Author

ThetaBird commented Jan 31, 2025

@dasfmi Thanks! I'm trying to get integration to pass (without adding monitors just yet) and am running into the following:

Image

Image

Command:

# values were redacted
AXIOM_TOKEN=xaat-token AXIOM_ORG_ID=personal-org pnpm run integration
# also tried adding  AXIOM_URL=https://api.axiom.co

The integration script was failing when process.env did not provide the values so I just added them when calling. I created a personal org with 0 datasets and created an API token with all possible dataset and org permissions checked. Still seeing Not Found & Bad Request. Are there any other steps that need to be taken to resolve these, or is something abnormally wrong on my end?

Also noticed that docs seem to be outdated so I'll create a separate PR once I get everything sorted out with this process.

@ThetaBird
Copy link
Author

@dasfmi

So this boils down to the default parallel test execution of vitest. Since each integration creates and deletes datasets before and after the run, race conditions are introduced where we try to create existing datasets and delete nonexisting ones. The tests pass in CI simply because we do max-parallel: 1 in test-integration.strategy in ci.yml.

I got things running locally without issues by changing the integration script in integration/package.json to:

 "integration": "vitest run --threads=false src/*"

Out of curiosity, how were things running locally on your end?

@ThetaBird
Copy link
Author

Ready for review in #265

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

No branches or pull requests

2 participants