-
Notifications
You must be signed in to change notification settings - Fork 199
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
Simplify metrics views #2515
Simplify metrics views #2515
Conversation
...g/src/main/java/io/opentelemetry/sdk/metrics/internal/view/PreAggregatedStandardMetrics.java
Outdated
Show resolved
Hide resolved
...g/src/main/java/io/opentelemetry/sdk/metrics/internal/view/PreAggregatedStandardMetrics.java
Outdated
Show resolved
Hide resolved
...g/src/main/java/io/opentelemetry/sdk/metrics/internal/view/PreAggregatedStandardMetrics.java
Outdated
Show resolved
Hide resolved
...g/src/main/java/io/opentelemetry/sdk/metrics/internal/view/PreAggregatedStandardMetrics.java
Outdated
Show resolved
Hide resolved
...g/src/main/java/io/opentelemetry/sdk/metrics/internal/view/PreAggregatedStandardMetrics.java
Outdated
Show resolved
Hide resolved
...g/src/main/java/io/opentelemetry/sdk/metrics/internal/view/PreAggregatedStandardMetrics.java
Outdated
Show resolved
Hide resolved
...opentelemetry/sdk/metrics/internal/view/PreAggregatedStandardMetricsAttributesProcessor.java
Outdated
Show resolved
Hide resolved
can we review this via meeting? |
agent/agent-tooling/src/main/java/io/opentelemetry/sdk/metrics/internal/view/MetricView.java
Outdated
Show resolved
Hide resolved
agent/agent-tooling/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistry.java
Outdated
Show resolved
Hide resolved
|
||
import io.opentelemetry.sdk.metrics.internal.view.AttributesProcessor; | ||
|
||
public class ViewBuilderAccessor { |
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.
should we move this to standalone exporter module so that it can be reused?
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.
I realized, I think it's too early to use in the standalone exporter module, because AttributesProcessor
is in an internal package, and we risk causing version conflicts for users if we use internal classes (which isn't the case in the javaagent since we have control of the SDK version used)
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.
are you going to propose to make AttributeProcessor public upstream in the future?
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.
added to this week's SIG meeting to discuss 👍
import java.util.Set; | ||
|
||
@SuppressWarnings("rawtypes") | ||
enum MetricView { |
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.
same here. move it to standalone exporter module?
private final Set<AttributeKey<?>> attributeKeys; | ||
private final boolean includeSynthetic; | ||
|
||
MetricView(String instrumentName, Set<AttributeKey<?>> attributeKeys, boolean includeSynthetic) { |
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.
MetricView(String instrumentName, Set<AttributeKey<?>> attributeKeys, boolean includeSynthetic) { | |
MetricView(String instrumentName, Set<AttributeKey<?>> attributeKeys, boolean hasSynthetic) { |
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.
renamed to captureSynthetic
|
||
String connectionString = readableSpan.getAttribute(AiSemanticAttributes.CONNECTION_STRING); | ||
if (connectionString != null) { | ||
builder.put(AiSemanticAttributes.CONNECTION_STRING, connectionString); |
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.
why do we need this for pre-agg?
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.
so pre-aggs get reported to the correct connection string when connectionStringOverrides
are used
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.
can we add a comment there mentioning connecitonStringOverrides?
agent/agent-tooling/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistry.java
Outdated
Show resolved
Hide resolved
HTTP_CLIENT_VIEW("http.client.duration", httpClientDurationAttributeKeys(), false), | ||
HTTP_SERVER_VIEW("http.server.duration", httpServerDurationAttributeKeys(), true), | ||
RPC_CLIENT_VIEW("rpc.client.duration", rpcClientDurationAttributeKeys(), false), | ||
RPC_SERVER_VIEW("rpc.server.duration", rpcServerDurationAttributeKeys(), false); |
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.
what about synthetic for rpc server?
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.
I realized that rpc instrumentation doesn't capture http.user_agent
No description provided.