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

feat(go-sdk): support OpenTelemetry metrics #428

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

Conversation

ewanharris
Copy link
Member

Description

Backports the telemetry changes back to the generator repository.

Also brings some actions versions changes and fixes and issue where we weren't correctly passing the consistency down for a batch check down to the individual checks.

References

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

dependabot bot and others added 4 commits October 9, 2024 10:51
Bumps the dependencies group with 1 update: [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action).


Updates `docker/setup-buildx-action` from 3.6.1 to 3.7.1
- [Release notes](https://github.com/docker/setup-buildx-action/releases)
- [Commits](docker/setup-buildx-action@988b5a0...c47758b)

---
updated-dependencies:
- dependency-name: docker/setup-buildx-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
...

Signed-off-by: dependabot[bot] <support@github.com>
Copy link
Member

@rhamzeh rhamzeh left a comment

Choose a reason for hiding this comment

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

All the changes in the dotnet/template/... can be dropped, these are build artifacts

| `http.request.method` | string | Yes | HTTP method for the request |
| `http.request.resend_count` | int | Yes | Number of retries attempted, if any |
| `http.response.status_code` | int | Yes | Status code of the response (e.g., `200` for success) |
| `http.server.request.duration` | int | Yes | Time taken by the FGA server to process and evaluate the request, in milliseconds |
Copy link
Member

Choose a reason for hiding this comment

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

This should be false by default, same for url.scheme, url.full and http.request.method 🤔

But those are less important - I'm not sure if the config matches the documentation, but it is crucial we do not enable http.server.request.duration as a default attribute b/c it has a very high cardinality

(Side-note: does not need to be in this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

I lifted this from one of the more recent SDKs (I think Python). Shall I just revert to whatever is on main currently in go-sdk?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed 08996ce to update the file with the table/preamble currently used on main in go-sdk.

// replace {{gitHost}}/{{gitUserId}}/{{gitRepoId}} v{{packageVersion}} => ../../
Copy link
Member

Choose a reason for hiding this comment

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

Also for a later PR - I think for all examples, we can make local the default

Copy link
Member Author

Choose a reason for hiding this comment

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

What dya think about eventually getting rid of things like {{gitHost}}/{{gitUserId}}/{{gitRepoId}} and things like {{appshortname}} in future too now that we have a single SDK publish target? 🥺

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.

2 participants