Skip to content

Simplify metrics views#2515

Merged
trask merged 6 commits into
mainfrom
simplify-metrics-views
Sep 14, 2022
Merged

Simplify metrics views#2515
trask merged 6 commits into
mainfrom
simplify-metrics-views

Conversation

@trask

@trask trask commented Sep 12, 2022

Copy link
Copy Markdown
Member

No description provided.

@heyams

heyams commented Sep 12, 2022

Copy link
Copy Markdown
Contributor

can we review this via meeting?


import io.opentelemetry.sdk.metrics.internal.view.AttributesProcessor;

public class ViewBuilderAccessor {

Copy link
Copy Markdown
Contributor

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?

@trask trask Sep 13, 2022

Copy link
Copy Markdown
Member Author

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)

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member Author

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 {

Copy link
Copy Markdown
Contributor

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
MetricView(String instrumentName, Set<AttributeKey<?>> attributeKeys, boolean includeSynthetic) {
MetricView(String instrumentName, Set<AttributeKey<?>> attributeKeys, boolean hasSynthetic) {

Copy link
Copy Markdown
Member Author

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);

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Contributor

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?

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);

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member Author

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

@trask trask merged commit 1a44b8d into main Sep 14, 2022
@trask trask deleted the simplify-metrics-views branch September 14, 2022 17:21
heyams pushed a commit that referenced this pull request Sep 16, 2022
heyams pushed a commit that referenced this pull request Sep 16, 2022
heyams pushed a commit that referenced this pull request Sep 16, 2022
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.

3 participants