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: enhancements to OpenTelemetry support #149

Merged
merged 30 commits into from
Sep 27, 2024

Conversation

evansims
Copy link
Member

@evansims evansims commented Aug 30, 2024

Description

⚠️ Breaking change ⚠️

This PR introduces enhancements to our OpenTelemetry metrics reporting support, migrating enhancements made to the Java, Go, and Python SDKs:

  • OpenTelemetry functionality is now configurable using the TelemetryConfiguration and TelemetryMetricConfigurationclasses.
  • FGA Method names are now title-cased, instead of camel-cased.
  • Configuration has been updated to support the acceptance of a TelemetryConfiguration instance for configuring metrics reporting at runtime.
  • Telemetry attributes and metric keys are now enums.
  • Adds unit tests for full coverage of our OpenTelemetry support.

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

@evansims evansims added the enhancement New feature or request label Aug 30, 2024
@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 88.95706% with 18 lines in your changes missing coverage. Please review.

Project coverage is 87.94%. Comparing base (09191e7) to head (be94b36).
Report is 57 commits behind head on main.

Files with missing lines Patch % Lines
api.ts 18.18% 0 Missing and 9 partials ⚠️
telemetry/configuration.ts 82.75% 2 Missing and 3 partials ⚠️
common.ts 88.88% 1 Missing and 1 partial ⚠️
telemetry/attributes.ts 96.15% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #149      +/-   ##
==========================================
- Coverage   91.22%   87.94%   -3.28%     
==========================================
  Files          19       23       +4     
  Lines        1003     1120     +117     
  Branches      133      171      +38     
==========================================
+ Hits          915      985      +70     
- Misses         82       83       +1     
- Partials        6       52      +46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@evansims evansims force-pushed the feat/enhance-opentelemetry-support branch from 3fe9b36 to bd78c29 Compare August 30, 2024 23:32
@evansims evansims marked this pull request as ready for review August 30, 2024 23:33
@evansims evansims requested a review from a team as a code owner August 30, 2024 23:33
package.json Show resolved Hide resolved
telemetry/attributes.ts Outdated Show resolved Hide resolved
@evansims evansims requested a review from rhamzeh August 31, 2024 00:34
@evansims evansims mentioned this pull request Aug 31, 2024
@rhamzeh
Copy link
Member

rhamzeh commented Sep 11, 2024

Quick thoughts on this -

My major concern is the configuration as that would be a breaking change to change later.

If we can move the attrib and metric names to enums and make the configuration something like here https://github.com/openfga/dotnet-sdk/blob/main/OpenTelemetry.md, that would be great

Having the constructor as it is currently, take a set of true/false is problematic, b/c it's not easy for the user to tell what they are configuring and we could easily do a breaking change by changing the order without the user being aware.

Validation for the config would be great too to help users avoid typos

For the record, here's what the config looks like in .NET

TelemetryConfig telemetryConfig = new () {
    Metrics = new Dictionary<string, MetricConfig> {
        [TelemetryMeters.TokenExchangeCount] = new () {
            Attributes = new HashSet<string> {
                TelemetryAttribute.HttpScheme,
                TelemetryAttribute.HttpMethod,
                TelemetryAttribute.HttpHost,
                TelemetryAttribute.HttpStatus,
                TelemetryAttribute.HttpUserAgent,
                TelemetryAttribute.RequestMethod,
                TelemetryAttribute.RequestClientId,
                TelemetryAttribute.RequestStoreId,
                TelemetryAttribute.RequestModelId,
                TelemetryAttribute.RequestRetryCount,
                TelemetryAttribute.ResponseModelId
            }
        },
        [TelemetryMeters.QueryDuration] = new () {
            Attributes = new HashSet<string> {
                TelemetryAttribute.HttpStatus,
                TelemetryAttribute.HttpUserAgent,
                TelemetryAttribute.RequestMethod,
                TelemetryAttribute.RequestClientId,
                TelemetryAttribute.RequestStoreId,
                TelemetryAttribute.RequestModelId,
                TelemetryAttribute.RequestRetryCount,
            }
        },
        [TelemetryMeters.QueryDuration] = new () {
            Attributes = new HashSet<string> {
                TelemetryAttribute.HttpStatus,
                TelemetryAttribute.HttpUserAgent,
                TelemetryAttribute.RequestMethod,
                TelemetryAttribute.RequestClientId,
                TelemetryAttribute.RequestStoreId,
                TelemetryAttribute.RequestModelId,
                TelemetryAttribute.RequestRetryCount,
            }
        },
    }
};

var configuration = new ClientConfiguration() {
    ApiUrl = Environment.GetEnvironmentVariable("FGA_API_URL"),
    StoreId = Environment.GetEnvironmentVariable("FGA_STORE_ID"),
    AuthorizationModelId = Environment.GetEnvironmentVariable("FGA_MODEL_ID"),
    // Credentials = ... // If needed
    Telemetry = telemetryConfig
};

Sidenote: would be good to update up the docs and example too

@jimmyjames jimmyjames marked this pull request as draft September 17, 2024 18:06
@jimmyjames jimmyjames force-pushed the feat/enhance-opentelemetry-support branch 3 times, most recently from 1d8efd9 to 1f7a00e Compare September 23, 2024 19:36
@jimmyjames jimmyjames force-pushed the feat/enhance-opentelemetry-support branch from 423222b to bbb7976 Compare September 24, 2024 00:01
@jimmyjames jimmyjames marked this pull request as ready for review September 24, 2024 17:49
@jimmyjames jimmyjames added this pull request to the merge queue Sep 27, 2024
Merged via the queue into main with commit 66ebf75 Sep 27, 2024
18 checks passed
@jimmyjames jimmyjames deleted the feat/enhance-opentelemetry-support branch September 27, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants