refactor: update OTLP Histogram Records #1754
Conversation
WalkthroughThis pull request primarily updates the version number of the Permify API from "v1.1.9" to "v1.2.0" across multiple files, including Swagger and OpenAPI documentation, as well as internal structures. Additionally, it simplifies the metrics handling in various components by removing duration-based measurements and focusing on counting operations instead. This change affects the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
tolgaOzen
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
internal/engines/cache/check.go (2)
28-28: LGTM! Good simplification of metrics.The consolidation to a single
cacheHitHistogramis a good architectural choice. Cache hit rate is typically the most important metric for cache performance analysis, making this a more focused approach.Consider adding cache miss tracking in the future if you need to calculate hit ratios or analyze cache effectiveness more deeply.
67-68: Enhance the comment clarity.The current comment could be more specific about what's being measured.
- // Increase the hit count in the metrics. + // Record a cache hit in the OTLP histogram metrics.internal/servers/schemaServer.go (2)
39-41: Fix incomplete metric description.The description for
write_schemahistogram ends with "in", which appears to be incomplete.- writeSchemaHistogram: telemetry.NewHistogram(internal.Meter, "write_schema", "amount", "Number of writing schema in"), + writeSchemaHistogram: telemetry.NewHistogram(internal.Meter, "write_schema", "amount", "Number of schema writes"),
39-41: Consider complementing operation counts with duration metrics.While the shift to operation counting provides valuable throughput metrics, consider maintaining both count and duration metrics for comprehensive observability. This would help in:
- Identifying performance bottlenecks
- Setting appropriate SLOs
- Monitoring system health
You could implement this using OpenTelemetry's
NewHistogramwith different units for each metric type.Also applies to: 84-84, 224-224, 249-249
internal/servers/dataServer.go (1)
Line range hint
50-413: Consider documenting the monitoring strategyThe shift from duration-based to count-based metrics represents a significant change in monitoring strategy. Consider:
- Documenting this change in monitoring documentation
- Updating dashboards and alerts that might depend on the removed duration metrics
- Ensuring observability requirements are still met with the new metrics
This will help maintain system observability and ensure smooth operations transition.
internal/invoke/invoke.go (1)
263-263: Consider adding metric attributes for consistency.While the Check method includes detailed attributes with its histogram recording, other methods (LookupEntity, LookupEntityStream, LookupSubject, SubjectPermission) record metrics without any context. Consider adding relevant attributes for better observability.
Example implementation for LookupEntity:
-invoker.lookupEntityHistogram.Record(ctx, 1) +invoker.lookupEntityHistogram.Record(ctx, 1, + metric.WithAttributeSet( + attribute.NewSet( + attribute.KeyValue{Key: "entity_type", Value: attribute.StringValue(request.GetEntityType())}, + attribute.KeyValue{Key: "subject_type", Value: attribute.StringValue(request.GetSubject().GetType())}, + )), +)Similar attribute sets should be added to other methods based on their context.
Also applies to: 304-304, 350-350, 396-396
internal/engines/cache/check_test.go (3)
44-44: Add parameter documentation for CheckEngineWithCache initialization.The struct initialization
CheckEngineWithCache{nil, nil, cache, nil}appears multiple times but lacks clear documentation about what each parameter represents. Consider adding comments to explain the purpose of each parameter for better maintainability.Also applies to: 94-94, 144-144, 288-288, 323-323
Line range hint
1-1200: Consider refactoring test setup for better maintainability.The test file has several areas that could be improved:
- Test data setup is duplicated across test cases. Consider extracting common setup code into helper functions.
- Cache configuration (
ristretto.New()) is repeated in each test. Consider creating a helper function or using BeforeEach hooks.- Some test cases have duplicate assertions that could be consolidated.
These changes would improve maintainability and reduce the likelihood of inconsistencies.
Line range hint
1-1200: Consider adding tests for additional scenarios.While the test coverage is comprehensive for permission checks, consider adding tests for:
- Cache eviction and invalidation scenarios
- Error cases for cache operations
- Concurrent access to the cache
These additional tests would help ensure the cache behaves correctly under all conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pkg/pb/base/v1/openapi.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
docs/api-reference/apidocs.swagger.json(1 hunks)docs/api-reference/openapiv2/apidocs.swagger.json(1 hunks)internal/engines/cache/check.go(3 hunks)internal/engines/cache/check_test.go(5 hunks)internal/info.go(1 hunks)internal/invoke/invoke.go(8 hunks)internal/servers/dataServer.go(8 hunks)internal/servers/schemaServer.go(4 hunks)proto/base/v1/openapi.proto(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- docs/api-reference/apidocs.swagger.json
- docs/api-reference/openapiv2/apidocs.swagger.json
- proto/base/v1/openapi.proto
🔇 Additional comments (13)
internal/info.go (2)
26-26: LGTM: Version bump follows semantic versioning.
The minor version increment (v1.1.9 → v1.2.0) appropriately reflects the internal improvements being made to the OTLP histogram recording system, as these changes add new functionality in a backward-compatible manner.
26-26: Verify version consistency across the codebase.
Let's ensure this version update is consistently applied across all relevant files.
✅ Verification successful
Version v1.2.0 is consistently applied across the codebase
The version string "v1.2.0" is consistently used across all relevant files:
internal/info.go: Version constantproto/base/v1/openapi.proto: API versiondocs/api-reference/apidocs.swagger.json: API documentationdocs/api-reference/openapiv2/apidocs.swagger.json: OpenAPI v2 documentation
No occurrences of the old version (v1.1.9) were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of the old version
# and verify the new version is consistently applied.
echo "Checking for any remaining occurrences of v1.1.9..."
rg "v1\.1\.9"
echo -e "\nVerifying v1.2.0 is consistently applied in key files..."
# Common locations for version strings
for pattern in "version.*1\.2\.0" "v1\.2\.0"
do
rg -g "*.{json,yaml,yml,proto,go}" "$pattern"
done
Length of output: 874
internal/engines/cache/check.go (2)
39-42: LGTM! Clear and well-structured initialization.
The metric initialization is clear with appropriate naming and units.
Line range hint 28-42: Verify telemetry consumers compatibility.
The change from duration-based to count-based metrics might affect downstream telemetry consumers (e.g., monitoring dashboards, alerts).
✅ Verification successful
Metric change is consistent with codebase patterns
The change from duration-based to count-based metrics aligns with the existing telemetry implementation:
- All histograms in the codebase use the same count-based pattern with "amount" unit
- The
telemetry.NewHistogramhelper consistently createsInt64Histogramwith count-based metrics - No duration-based histograms were found in the codebase
- Metric export configuration in
example.config.yamlis generic and not tied to specific metric types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining duration-based histogram references
echo "Checking for any remaining duration histogram references..."
rg -i "duration.*histogram" --type go
echo "Checking for potential metric consumers..."
rg -l "cache_hit_duration" --type yaml --type json
Length of output: 314
Script:
#!/bin/bash
# Let's try a broader search to understand the telemetry setup and consumers
# Check for metric-related configurations and consumers
echo "Checking for metric configurations..."
rg -i "metric|telemetry" --type yaml --type json -A 5
# Look for other histogram usages to understand the pattern
echo "Checking histogram usage patterns..."
rg -i "histogram" --type go -A 2
# Check for monitoring or observability related files
echo "Looking for monitoring configurations..."
fd -e yaml -e yml -e json . | grep -i "monitor\|metric\|telemetry\|observ"
# Search for the specific metric name
echo "Searching for cache_hit metric usage..."
rg "cache_hit" --type go --type yaml --type json -A 2
Length of output: 9136
internal/servers/schemaServer.go (3)
84-84: LGTM!
The histogram recording is correctly placed after error handling and records successful schema writes.
224-224: LGTM!
The histogram recording is correctly placed after error handling and records successful schema reads.
249-249: LGTM!
The histogram recording is correctly placed after both error checks and records successful schema listings.
internal/servers/dataServer.go (2)
101-101: LGTM: Consistent metric recording pattern
The histogram recording points are consistently placed:
- After successful operation completion
- Before returning the response
- Using a constant value of 1 for counting
This ensures accurate tracking of successful operations.
Also applies to: 150-150, 248-248, 315-315, 345-345, 375-375, 413-413
50-56: Consider the impact of removing duration metrics
While switching from duration to count-based metrics simplifies the monitoring, it removes visibility into operation latency. This could make it harder to:
- Identify performance bottlenecks
- Set meaningful SLOs
- Debug timing-related issues
Consider maintaining both metrics types or ensuring alternative latency monitoring is in place.
internal/invoke/invoke.go (4)
9-9: LGTM: Import addition is appropriate.
The added metric import aligns with the refactoring to use histograms for metrics collection.
89-99: LGTM: Constructor updates maintain consistency.
The histogram initializations follow a consistent pattern with clear metric names and descriptions. The use of "amount" as the unit type is appropriate for counting operations.
114-120: LGTM: Well-structured metric recording with context.
The Check method's histogram recording includes relevant attributes that provide valuable context for metrics analysis.
71-74: LGTM: Histogram field changes align with metrics refactoring.
The new histogram fields appropriately replace the removed counters and duration histograms, maintaining a consistent naming pattern.
✅ Verification successful
The previous search might have been too specific. Let's broaden it to catch any remaining references to the old metric types and verify the changes are complete.
The previous results showed mostly unrelated matches. Let's focus specifically on the metrics implementation in the file under review.
The duration histograms have been successfully replaced with amount histograms
The verification confirms that:
- All old duration histograms have been removed from the file
- New Int64Histogram fields are properly implemented
- No remaining references to the old metric types exist in the file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all duration histograms have been removed and replaced with amount histograms
rg -l "DurationHistogram|Counter.*api\.Int64Counter" .
Length of output: 56
Script:
#!/bin/bash
# Search for any remaining duration or counter metrics in the codebase
rg -i "duration.*histogram|counter" .
# Also check for any metric registrations or initializations
ast-grep --pattern 'NewInt64(Counter|DurationHistogram)'
Length of output: 24817
Script:
#!/bin/bash
# Let's check the specific file for any old metric-related code and the new histogram implementations
rg -A 2 "Int64(Counter|DurationHistogram|Histogram)" internal/invoke/invoke.go
# Also check for metric initialization in this file
ast-grep --pattern 'NewInt64$_($_, $_)'
Length of output: 530
Description:
This PR refactors the OTLP histogram recording logic.
Changes:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation