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

Athena support #473

Closed
wants to merge 3 commits into from
Closed

Athena support #473

wants to merge 3 commits into from

Conversation

rabidaudio
Copy link

Addresses #472

Here's a first pass. It will need some documentation and cleanup, and also I haven't been able to get the test to run locally for some reason.

@changeset-bot
Copy link

changeset-bot bot commented Nov 20, 2022

⚠️ No Changeset found

Latest commit: df10fa6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Nov 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
evidence-docs ✅ Ready (Inspect) Visit Preview Nov 20, 2022 at 6:13PM (UTC)

@rabidaudio
Copy link
Author

I'm trying to use this fork locally in my project, but there's some pnpm magic happening here I don't totally understand.

I had to downgrade to npm version 8.19.3 because it seems that npm 9 removed support for the workspace: thing?

  1. I tried npm i /path/to/evidence/package/evidence. This doesn't error, but doesn't install any of the peer dependencies such as svelte, resulting in
> selfdata@0.0.1 dev
> evidence dev -o

npm ERR! could not determine executable to run
  1. Tried npm i /path/to/evidence. This results in npm ERR! Cannot read properties of undefined (reading 'spec')

  2. Tried 1. but also manually install all the peer dependencies. Then the server started, but svelte dependencies like svelte-tiny-linked-charts couldn't be found

@archiewood
Copy link
Member

Hey!

This looks like such an epic PR.

I really like the idea of adding Athena. I haven’t used it before but it seems very cool.

One of the things that we’re about at Evidence is having a really lightweight tool that doesn’t take time and pain to set up.

If I’m understanding it right, Athena is exciting to me as it makes it possible for the database side of running your analytics to also be lightweight.

@archiewood
Copy link
Member

Re testing does this help ?

@rabidaudio
Copy link
Author

I was able to prove it was working using the example project, but I haven't figured out how to use my forked branch in my real project.

But maybe it doesn't matter? If you're keen on getting this merged, let me know what else is needed to get there

@ud3sh
Copy link
Member

ud3sh commented Nov 25, 2022

I was able to prove it was working using the example project, but I haven't figured out how to use my forked branch in my real project.

But maybe it doesn't matter? If you're keen on getting this merged, let me know what else is needed to get there

@rabidaudio if it works on the example project, you can consider it working. Thanks,

@archiewood
Copy link
Member

@rabidaudio

Re: dev issues

I'm trying to use this fork locally in my project, but there's some pnpm magic happening here I don't totally understand.

I haven't been able to get the test to run locally for some reason.

Your difficulties above are due to some details of how Evidence is currently implemented, which make it harder than it should be to add extensions. We want to work on this in pretty short order (see #486), which should make community contributions easier.

Re: feature itself

We think this is a really nice feature. Particularly as Athena supports both csv and Parquet, which are often a part of the analyst toolkit.

However, we're a bit tentative about including this in the core package:

  • As a reasonably technical analyst myself, after c.1hr messing around in AWS and knee deep in their docs, I still haven't managed to successfully set up Athena
  • I suspect (but could be wrong) that this would be used by a relatively small proportion of Evidence users

As such, we'd be interested in making this one of our first "community supported" connectors, which would effectively mean you'd be publishing a separate package to npm.

Once we have finished with #486, if we were to provide good docs to do this (not yet written), would you be open to this?

@rabidaudio
Copy link
Author

As a reasonably technical analyst myself, after c.1hr messing around in AWS and knee deep in their docs, I still haven't managed to successfully set up Athena

Yeah, everything in AWS is pretty user-unfriendly, and Athena particularly so. I set it up through a combination of Terraform and relying on target-s3-parquet, I'm not sure I would know how to do it through the web console.

As such, we'd be interested in making this one of our first "community supported" connectors, which would effectively mean you'd be publishing a separate package to npm.

Once we have finished with #486, if we were to provide good docs to do this (not yet written), would you be open to this?

@archiewood Yeah absolutely. I probably would have gone this route to begin with if it were available. Let me know if I can help, maybe kick the tires of a #486 solution before it's fully documented

@archiewood
Copy link
Member

Okay, I'm going to close this, as I think the implementation will be meaningfully different from this. We can keep track in #472

@archiewood archiewood closed this Dec 7, 2022
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.

None yet

3 participants