-
Notifications
You must be signed in to change notification settings - Fork 20
feat(opa-gson): add MetricsTypeAdapterFactory mirroring opa-jackson #59 #72
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| package io.github.open_policy_agent.opa.gson; | ||
|
|
||
| import com.google.gson.Gson; | ||
| import com.google.gson.TypeAdapter; | ||
| import com.google.gson.TypeAdapterFactory; | ||
| import com.google.gson.reflect.TypeToken; | ||
| import com.google.gson.stream.JsonReader; | ||
| import com.google.gson.stream.JsonWriter; | ||
| import io.github.open_policy_agent.opa.metrics.Metrics; | ||
| import java.io.IOException; | ||
|
|
||
| /** | ||
| * Gson {@link TypeAdapterFactory} that serializes {@link Metrics} types to match the JSON shape | ||
| * produced by OPA's decision logs ({@code timer_<name>_ns} convention). | ||
| * | ||
| * <ul> | ||
| * <li>{@link Metrics.Timer} → nanoseconds (long) | ||
| * <li>{@link Metrics.Counter} → integer value | ||
| * <li>{@link Metrics.Histogram} → its {@link Metrics.Histogram.Values} object | ||
| * </ul> | ||
| * | ||
| * <p>Usage: | ||
| * <pre>{@code | ||
| * Gson gson = new GsonBuilder() | ||
| * .registerTypeAdapterFactory(new MetricsTypeAdapterFactory()) | ||
| * .create(); | ||
| * String json = gson.toJson(timer); | ||
| * }</pre> | ||
| */ | ||
| public class MetricsTypeAdapterFactory implements TypeAdapterFactory { | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| @Override | ||
| public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) { | ||
| if (Metrics.Timer.class.isAssignableFrom(type.getRawType())) { | ||
| return (TypeAdapter<T>) new TimerAdapter(); | ||
| } | ||
| if (Metrics.Counter.class.isAssignableFrom(type.getRawType())) { | ||
| return (TypeAdapter<T>) new CounterAdapter(); | ||
| } | ||
| if (Metrics.Histogram.class.isAssignableFrom(type.getRawType())) { | ||
| return (TypeAdapter<T>) new HistogramAdapter(gson); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private static final class TimerAdapter extends TypeAdapter<Metrics.Timer> { | ||
| @Override | ||
| public void write(JsonWriter out, Metrics.Timer timer) throws IOException { | ||
| if (timer == null) { | ||
| out.nullValue(); | ||
| } else { | ||
| out.value(timer.value().toNanos()); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Metrics.Timer read(JsonReader in) { | ||
| throw new UnsupportedOperationException("Deserialization of Metrics.Timer is not supported"); | ||
| } | ||
| } | ||
|
|
||
| private static final class CounterAdapter extends TypeAdapter<Metrics.Counter> { | ||
| @Override | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Consider adding one test per adapter using a small hand-built |
||
| public void write(JsonWriter out, Metrics.Counter counter) throws IOException { | ||
| if (counter == null) { | ||
| out.nullValue(); | ||
| } else { | ||
| out.value(counter.value()); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Metrics.Counter read(JsonReader in) { | ||
| throw new UnsupportedOperationException( | ||
| "Deserialization of Metrics.Counter is not supported"); | ||
| } | ||
| } | ||
|
|
||
| private static final class HistogramAdapter extends TypeAdapter<Metrics.Histogram> { | ||
| private final TypeAdapter<Metrics.Histogram.Values> valuesAdapter; | ||
|
|
||
| HistogramAdapter(Gson gson) { | ||
| this.valuesAdapter = gson.getAdapter(Metrics.Histogram.Values.class); | ||
| } | ||
|
|
||
| @Override | ||
| public void write(JsonWriter out, Metrics.Histogram histogram) throws IOException { | ||
| if (histogram == null) { | ||
| out.nullValue(); | ||
| } else { | ||
| valuesAdapter.write(out, histogram.value()); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Metrics.Histogram read(JsonReader in) { | ||
| throw new UnsupportedOperationException( | ||
| "Deserialization of Metrics.Histogram is not supported"); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| package io.github.open_policy_agent.opa.gson; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import com.google.gson.Gson; | ||
| import com.google.gson.GsonBuilder; | ||
| import io.github.open_policy_agent.opa.metrics.Metrics; | ||
| import io.github.open_policy_agent.opa.metrics.SimpleMetrics; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| /** | ||
| * Parallel to opa-jackson's MetricsModuleTest. Verifies that MetricsTypeAdapterFactory produces | ||
| * JSON matching OPA's decision log shape ({@code timer_<name>_ns} = nanoseconds as a long). | ||
| */ | ||
| class MetricsTypeAdapterFactoryTest { | ||
|
|
||
| private final Gson gson = | ||
| new GsonBuilder().registerTypeAdapterFactory(new MetricsTypeAdapterFactory()).create(); | ||
|
|
||
| @Test | ||
| void timer_serializesAsNanoseconds() { | ||
| SimpleMetrics metrics = new SimpleMetrics(); | ||
| Metrics.Timer timer = metrics.timer("rego_query_eval"); | ||
| timer.start(); | ||
| timer.stop(); | ||
|
|
||
| String json = gson.toJson(timer, Metrics.Timer.class); | ||
|
|
||
| // Must be a plain number (nanoseconds), matching DecisionLogPlugin's timer.value().toNanos() | ||
| long nanos = timer.value().toNanos(); | ||
| assertThat(json).isEqualTo(String.valueOf(nanos)); | ||
| } | ||
|
|
||
| @Test | ||
| void timer_jsonShapeMatchesDirectNanosSerialization() { | ||
| SimpleMetrics metrics = new SimpleMetrics(); | ||
| Metrics.Timer timer = metrics.timer("rego_query_eval"); | ||
| timer.start(); | ||
| timer.stop(); | ||
|
|
||
| String timerJson = gson.toJson(timer, Metrics.Timer.class); | ||
| String nanosJson = gson.toJson(timer.value().toNanos()); | ||
|
|
||
| // Serializing the Timer must produce exactly what serializing its nanosecond value produces. | ||
| assertThat(timerJson).isEqualTo(nanosJson); | ||
| } | ||
|
|
||
| @Test | ||
| void timer_null_serializesAsNull() { | ||
| String json = gson.toJson(null, Metrics.Timer.class); | ||
| assertThat(json).isEqualTo("null"); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timer JSON shape diverges from the opa-jackson
MetricsModulethis mirrors.This emits nanoseconds as an integer (
12345), but opa-jackson serializesMetrics.Timeras itsDurationvia@JsonValue, which withJavaTimeModuleproduces fractional seconds — a 12345 ns timer yields0.000012345. So a consumer who swaps mappers gets a structurally different value for the same timer, which cuts against the "close the parity gap" goal of the PR.The nanosecond shape here is the correct one: it matches
DecisionLogPlugin(timer.value().toNanos()) and OPA's canonical decision-log format, where thetimer_<name>_nskey declares the unit as nanoseconds. jackson's fractional-seconds output is effectively a unit mismatch under that key, so opa-jackson is the module that should change — not this one.So: keep this gson adapter as written, and align
opa-jackson'sMetricsModuleto emit nanoseconds too (e.g. aJsonSerializer<Metrics.Timer>or a@JsonValuemethod returningvalue().toNanos(), plus updatingMetricsModuleTestto assert againsttoNanos()). That also drops theJavaTimeModulerequirement on that path.@sanajitjana if you have the time, would you mind folding that jackson fix into this PR so parity is actually true when it lands? No pressure — if you'd rather keep this PR scoped to gson, I'll follow up with the jackson change separately.