Skip to content

feat(telemetry server): create Go Telemetry server and update cli to env #1

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

fishonamos
Copy link

@fishonamos fishonamos commented May 30, 2025

This PR enhances the telemetry server with several key features for improved usability, flexibility, and deployment.

Closes pyrevitlabs/pyRevit#2157

Server Updates (Read/Search Endpoints)

How to Run

# Build the server
go build -o pyrevit-telemetryserver

# Run with environment variables
PYREVT_TELEMETRY_DB_CONNSTRING="mongodb://localhost:27017/atoma-test" \
PYREVT_TELEMETRY_SCRIPTS_TABLE="scripts" \
PYREVT_TELEMETRY_EVENTS_TABLE="events" \
./pyrevit-telemetryserver --port=8080
  • Added GET endpoints for reading and searching telemetry data:

    • /api/v1/scripts/
    • /api/v2/scripts/
    • /api/v2/events/
  • These endpoints support pagination and query filtering for easier data retrieval.

  • Backend support added for both SQL and MongoDB.

  • Minor code cleanups and fixes (e.g., linter issues, missing imports, and type errors).

Environment Variable Support

  • CLI flags (e.g., --scripts, --events, --port, --debug, --trace) can now be overridden with environment variables like:

    • PYREVT_TELEMETRY_SCRIPTS_TABLE
    • PYREVT_TELEMETRY_EVENTS_TABLE
    • PYREVT_TELEMETRY_PORT
    • PYREVT_TELEMETRY_DB_CONNSTRING
  • CLI flags still take precedence over env vars if both are set.

Testing

  • I verified the following endpoints using curl and environment variables:

    • GET /api/v1/status
    • GET /api/v1/scripts/
    • GET /api/v2/scripts/
    • GET /api/v2/events/
    • POST /api/v1/scripts/
    • POST /api/v2/events/
    • Server starts and listens on the correct port.
    • Requests return expected responses (e.g., JSON payloads, appropriate error messages like "Invalid session ID").

@fishonamos fishonamos marked this pull request as ready for review May 30, 2025 09:27
@fishonamos fishonamos marked this pull request as draft May 30, 2025 11:08
@jmcouffin
Copy link

pyrevitlabs/pyRevit#2157

@fishonamos fishonamos marked this pull request as ready for review May 30, 2025 19:29
@fishonamos
Copy link
Author

@jmcouffin This first pr is ready for review. I want to break the pr into two parts. The second pr will take care of the docker setup, general refactor, (and Grafana?)

@dosymep
Copy link
Member

dosymep commented Jun 2, 2025

@fishonamos please add github actions ci\cd script, maybe it's worth adding a script for docker with github actions.

example configuration for go build (I recommend checking the script, I wrote it quickly):

name: main

on:
  workflow_dispatch:

  push:
    branches: [ main ]

jobs:
  build:
    runs-on: windows-latest
    steps:
      - uses: actions/checkout@v4

      # Setup go env: https://github.com/actions/setup-go
      - name: Setup Go
        uses: actions/setup-go@v5

      - name: Build
      - run: go build -o bin/pyrevit-telemetryserver
      
      # Upload docs site: https://github.com/marketplace/actions/upload-a-build-artifact
      - name: Upload Artifacts
        uses: actions/upload-artifact@v4
        with:
          name: bin
          path: bin

@fishonamos
Copy link
Author

@fishonamos please add github actions ci\cd script, maybe it's worth adding a script for docker with github actions.

example configuration for go build (I recommend checking the script, I wrote it quickly):

name: main

on:
  workflow_dispatch:

  push:
    branches: [ main ]

jobs:
  build:
    runs-on: windows-latest
    steps:
      - uses: actions/checkout@v4

      # Setup go env: https://github.com/actions/setup-go
      - name: Setup Go
        uses: actions/setup-go@v5

      - name: Build
      - run: go build -o bin/pyrevit-telemetryserver
      
      # Upload docs site: https://github.com/marketplace/actions/upload-a-build-artifact
      - name: Upload Artifacts
        uses: actions/upload-artifact@v4
        with:
          name: bin
          path: bin

Yes. Let me push the build.

@fishonamos
Copy link
Author

Updated @dosymep

Copy link
Collaborator

@sanzoghenzo sanzoghenzo left a comment

Choose a reason for hiding this comment

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

I realize that I should have moved the original code here before this PR 😅

Code changes seems fine, I threw some suggestions, but they can be done in future refactorings.

One great addition would be turning the curl tests you mentioned into unit tests with httptest; and maybe also tests on the config/envvars/cli flags part.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think we could use a library like config to handle config, envs and cli args all at once declaratively?

This should also remove the need of the usage.go module

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps a note for later... could we use slog here?

Copy link
Author

@fishonamos fishonamos Jun 2, 2025

Choose a reason for hiding this comment

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

Yes. We could do that in the next refactor pr. Slog will provide a more structured logging. I will note it in the list of next updates to make. I can do it

Copy link
Author

Choose a reason for hiding this comment

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

we should do that in a separate pr after this is merged? @sanzoghenzo because this already has too many changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, let's save it for later.

I'll create new issues from these comments (and move/split the original issue from pyrevit repo)

Copy link
Author

Choose a reason for hiding this comment

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

Great!

@sanzoghenzo
Copy link
Collaborator

One great addition would be turning the curl tests you mentioned into unit tests with httptest; and maybe also tests on the config/envvars/cli flags part.

@fishonamos are you able to do that, so then we can kickstart the project?

@fishonamos
Copy link
Author

One great addition would be turning the curl tests you mentioned into unit tests with httptest; and maybe also tests on the config/envvars/cli flags part.

@fishonamos are you able to do that, so then we can kickstart the project?

Yes. But we need to merge this first, then i will branch out and start working on it.

@sanzoghenzo
Copy link
Collaborator

One great addition would be turning the curl tests you mentioned into unit tests with httptest; and maybe also tests on the config/envvars/cli flags part.

@fishonamos are you able to do that, so then we can kickstart the project?

Yes. But we need to merge this first, then i will branch out and start working on it.

Why? I would rather add tests in this pr/branch to ensure everything is working, then start to work on the other features

@fishonamos
Copy link
Author

One great addition would be turning the curl tests you mentioned into unit tests with httptest; and maybe also tests on the config/envvars/cli flags part.

@fishonamos are you able to do that, so then we can kickstart the project?

Yes. But we need to merge this first, then i will branch out and start working on it.

Why? I would rather add tests in this pr/branch to ensure everything is working, then start to work on the other features

Oh. I think I understand you now. You mean the endpoints I added in the description right? Definitely! they can be added. I will do that and push.

@sanzoghenzo
Copy link
Collaborator

Hi @fishonamos ! I totally forgot about this PR... is it ready for review?

@fishonamos
Copy link
Author

Hi @fishonamos ! I totally forgot about this PR... is it ready for review?

Yes. I sent you a dm. I think you missed it.

@sanzoghenzo
Copy link
Collaborator

Hi @fishonamos ! I totally forgot about this PR... is it ready for review?

Yes. I sent you a dm. I think you missed it.

Oh, sorry for that... where did you sent it? I cannot find anything on discourse, but it might be that I deleted/ignored it because of the many moderation messages I got 😅

Anyway, here it comes my review...

Copy link
Collaborator

@sanzoghenzo sanzoghenzo left a comment

Choose a reason for hiding this comment

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

Here you are!

Comment on lines +34 to +56
docker:
runs-on: ubuntu-latest
needs: build
steps:
- uses: actions/checkout@v4

- name: Set up QEMU
uses: docker/setup-qemu-action@v3

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3

- name: Login to DockerHub # TODO: uncomment if pushing
# uses: docker/login-action@v3
# with:
# username: ${{ secrets.DOCKERHUB_USERNAME }}
# password: ${{ secrets.DOCKERHUB_TOKEN }}

- name: Build Docker image
run: docker build -t pyrevit-telemetryserver:latest .

# - name: Push Docker image # Will be uncommented if pushing
# run: docker push pyrevit-telemetryserver:latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

This job should run only on "release" and sync the docker tag with the version.
Since this PR doesn't have a Dockerfile, it is better to remove this job and think about it later.

Comment on lines +31 to +32
- name: Run Tests
run: go test -v ./...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test should be run before the build to ensure everything is OK

Comment on lines +57 to +72
func TestStatusEndpoint(t *testing.T) {
router := setupTestRouter()

t.Run("GET /api/v1/status returns 200", func(t *testing.T) {
req := httptest.NewRequest("GET", "/api/v1/status", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, req)

if w.Code != http.StatusOK {
t.Errorf("expected 200 OK, got %d", w.Code)
}
if w.Body.Len() == 0 {
t.Errorf("expected non-empty body")
}
})
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it is overkill for this simple endpoint, but there's also the /api/v2/status endpoint. Since it is identical, a for loop of "v1" and "v2" should cover everything.

- name: Setup Go
uses: actions/setup-go@v5
with:
go-version: "1.22"
Copy link
Collaborator

Choose a reason for hiding this comment

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

go.mod uses: "go 1.23.0" and "toolchain go1.24.2", should this variable reflect that?

Comment on lines +65 to +70
if scriptTable == "" {
scriptTable = os.Getenv("PYREVT_TELEMETRY_SCRIPTS_TABLE")
}
if eventTable == "" {
eventTable = os.Getenv("PYREVT_TELEMETRY_EVENTS_TABLE")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is another ranting for the future: do we really need to have dynamic table names? Especially now that you're introducing the APIs to get the data out of the DB....


## API Endpoints

- `GET /api/v1/status` — Health check
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is also the /api/v2/status endpoint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Telemetry Server
4 participants