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.

Sign up for GitHub

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

Jump to bottom

Make the OpenTelemetry InputFormat More Flexible to Metric, Value and Attribute Types #67

Merged
merged 5 commits into from
Feb 25, 2022
Merged

Make the OpenTelemetry InputFormat More Flexible to Metric, Value and Attribute Types #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5aa7f9c
Quietly ignore histogram and summary metrics
marcusgreer Feb 9, 2022
91916a7
use Collections::emtpyList to prevent memory allocation
marcusgreer Feb 9, 2022
6bf90a2
adding the OpenTelemetry Extension DruidModule
marcusgreer Feb 23, 2022
5be3c14
allow the OpenTelemetryProtobufReader to have attribute values of any…
marcusgreer Feb 25, 2022
2b6709a
Do not throw an error when the DataPoint Value is VALUE_NOT_SET
marcusgreer Feb 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 28 additions & 9 deletions ...rg/apache/druid/data/input/opentelemetry/protobuf/OpenTelemetryMetricsProtobufReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.apache.druid.java.util.common.parsers.ParseException;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -94,7 +95,7 @@ private List<InputRow> parseMetricsData(final MetricsData metricsData)
.getAttributesList()
.stream()
.collect(Collectors.toMap(kv -> resourceAttributePrefix + kv.getKey(),
kv -> getStringValue(kv.getValue())));
kv -> parseAnyValue(kv.getValue())));
return resourceMetrics.getInstrumentationLibraryMetricsList()
.stream()
.flatMap(libraryMetrics -> libraryMetrics.getMetricsList()
Expand Down Expand Up @@ -124,6 +125,11 @@ private List<InputRow> parseMetric(Metric metric, Map<String, Object> resourceAt
break;
}
// TODO Support HISTOGRAM and SUMMARY metrics
case HISTOGRAM:
case SUMMARY: {
inputRows = Collections.emptyList();
break;
}
default:
throw new IllegalStateException("Unexpected value: " + metric.getDataCase());
}
Expand All @@ -143,25 +149,38 @@ private InputRow parseNumberDataPoint(NumberDataPoint dataPoint,

if (dataPoint.hasAsInt()) {
event.put(valueDimension, dataPoint.getAsInt());
} else if (dataPoint.hasAsDouble()) {
event.put(valueDimension, dataPoint.getAsDouble());
} else {
throw new IllegalStateException("Unexpected dataPoint value type. Expected Int or Double");
event.put(valueDimension, dataPoint.getAsDouble());
}

event.putAll(resourceAttributes);
dataPoint.getAttributesList().forEach(att -> event.put(metricAttributePrefix + att.getKey(),
getStringValue(att.getValue())));
parseAnyValue(att.getValue())));

return createRow(TimeUnit.NANOSECONDS.toMillis(dataPoint.getTimeUnixNano()), event);
}

private static String getStringValue(AnyValue value)
private static Object parseAnyValue(AnyValue value)
{
if (value.getValueCase() == AnyValue.ValueCase.STRING_VALUE) {
return value.getStringValue();
switch (value.getValueCase()) {
case INT_VALUE:
return value.getIntValue();
case BOOL_VALUE:
return value.getBoolValue();
case ARRAY_VALUE:
return value.getArrayValue();
Copy link
Member

Choose a reason for hiding this comment

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

how does this get treated?

Copy link
Author

Choose a reason for hiding this comment

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

Does druid have a set of expected value types for columns? If it does, it would probably make sense for us to do some conversion from the disallowed types into strings or some other type.

Copy link

@freyayunfu freyayunfu Feb 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

just fyi, for converter, we only support int, bool , double and string here: https://github.com/confluentinc/opencensus-protobuf-converter/blob/d96358a0d61f6f3524c3acc9d7db264a7a1a34c8/src/main/java/io/confluent/connect/opentelemetry/OpenTelemetryConverter.java#L91-L93 Not sure whether we want to keep it consistent here. (sorry for merging the PR without noticing the difference here.)

Copy link
Author

@marcusgreer marcusgreer Feb 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like the OpenCensusProtobufProtobufReader changes all values into strings (LabelValue::getValue here returns a String). So I'll do the same thing here.

Copy link

Choose a reason for hiding this comment

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

the link i provided was for attributes, not metrics value. This should be the correct one: https://github.com/confluentinc/opencensus-protobuf-converter/blob/d96358a0d61f6f3524c3acc9d7db264a7a1a34c8/src/main/java/io/confluent/connect/opentelemetry/OpenTelemetryMetricsConverter.java#L121-L166

case BYTES_VALUE:
return value.getBytesValue();
case DOUBLE_VALUE:
return value.getDoubleValue();
case KVLIST_VALUE:
return value.getKvlistValue();
case STRING_VALUE:
return value.getStringValue();
default:
// VALUE_NOT_SET:
return "";
Copy link
Member

Choose a reason for hiding this comment

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

how does empty string get treated if someone uses a numeric column?

Copy link

@freyayunfu freyayunfu Feb 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It will return 0 as a default value of numeric column. In the code, here is what happens in detail: it would throw below exception first from here

 throw new NumberFormatException("empty String");

and then get caught here, and return default value.

Probably it's still better to return null here since druid would replace with default value according to different column specs directly here instead of going throw the whole parsing process, even though the eventual result is the same.

}
throw new IllegalStateException("Unexpected value: " + value.getValueCase());
}

InputRow createRow(long timeUnixMilli, Map<String, Object> event)
Expand Down
16 changes: 16 additions & 0 deletions ...tensions/src/main/resources/META-INF/services/org.apache.druid.initialization.DruidModule
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
# the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

org.apache.druid.data.input.opencensus.protobuf.OpenCensusProtobufExtensionsModule
Copy link
Member

Choose a reason for hiding this comment

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

should this be OpenTelemetry?