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.

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

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

Merged
merged 5 commits into from
Feb 25, 2022

Conversation

marcusgreer
Copy link

@marcusgreer marcusgreer commented Feb 9, 2022

Fixes #63

Description

  • Quietly ignore histogram and summary metrics in the OpenTelemetry InputFormat. Previously an exception would be thrown if a histogram or summary metric were ingested.
  • Do not throw an error if DataPoint values are VALUE_NOT_SET, instead use a double of value 0.
  • Do not throw an error if Metric Attributes are not strings, instead parse the object based off of the valueCase. If VALUE_NOT_SET, then set to an empty string
  • Create a DruidModule because the OpenTelemetryInputFormat could not be deployed as a standalone InputFormat since Druid could not discover the OpenTelemetryExtensionModule.

@marcusgreer marcusgreer requested a review from xvrl February 9, 2022 19:45
@@ -124,6 +124,11 @@ public OpenTelemetryMetricsProtobufReader(
break;
}
// TODO Support HISTOGRAM and SUMMARY metrics
case HISTOGRAM:
case SUMMARY: {
inputRows = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

instead of creating a new arraylist, we should return an empty list, which avoids unnecessary memory allocation

Suggested change
inputRows = new ArrayList<>();
inputRows = Collections.emptyList();

xvrl
xvrl previously approved these changes Feb 9, 2022
@marcusgreer marcusgreer changed the title Quietly Ignore Histogram and Summary Metrics in OpenTelemetry InputFormat Make the OpenTelemetry InputFormat More Flexible to value and attribute Inputs Feb 25, 2022
Copy link

@freyayunfu freyayunfu left a comment

Choose a reason for hiding this comment

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

lgtm thanks for fixing this!

@marcusgreer marcusgreer changed the title Make the OpenTelemetry InputFormat More Flexible to value and attribute Inputs Make the OpenTelemetry InputFormat More Flexible to Metric, Value and Attribute Types Feb 25, 2022
@freyayunfu freyayunfu merged commit 1a09b2c into 0.21.0-confluent Feb 25, 2022
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

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

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.

Choose a reason for hiding this comment

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

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

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.

# 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?

harinirajendran pushed a commit that referenced this pull request Mar 1, 2022
kkonstantine pushed a commit that referenced this pull request Sep 1, 2022
kkonstantine pushed a commit that referenced this pull request Sep 1, 2022
kkonstantine pushed a commit that referenced this pull request Oct 11, 2022
kkonstantine referenced this pull request in kkonstantine/druid Oct 26, 2022
kkonstantine referenced this pull request in kkonstantine/druid Oct 27, 2022
kkonstantine pushed a commit that referenced this pull request Dec 13, 2022
kkonstantine pushed a commit that referenced this pull request Jan 3, 2023
m-ghazanfar pushed a commit that referenced this pull request May 29, 2023
m-ghazanfar pushed a commit that referenced this pull request Jun 6, 2023
pagrawal10 added a commit that referenced this pull request Dec 18, 2023
* Bring dockerfile up to date

* add opencensus extension

* make checkstyle happy

* bump pom version for opencensus extension

* fix issues related to shading opencensus extension

The extension packaging included both shaded and unshaded dependencies
in the classpath. Shading should not be necessary in this case.

Also excludes guava dependencies, which are already provided by Druid
and don't need to be added to the extensions jars.

* METRICS-516: Adding Resource labels in OpenCensus Extension

* bump extension version to match release

* confluent-extensions with custom transform specs (#9)

* fix extraction transform serde (#10)

* fix check-style build errors

* setup semaphore build

* add checkstyle

* fix edge cases for internal topics

* METRICS-1302: Added prefix support for resource labels. (#14)

* METRICS-1302: Added prefix support for resource labels.

* Addressed review comments.

* Added and moved configs to ingestion spec, optimized code.

* Addressed review comments

* Updated metric dimesnion and other review comments

* Flipped ternary operator

* Moved from NullHandling to StringUtils.

* Removed unnecessary HashMap.

* Removed verbosity for instance variables.

* Added getters for configs, labels for distribution metric. (#15)

* Added getters for configs, labels for distribution metric.

* Addressed review comments

* Removed extra brackets in JsonProperty.

* Default resource label prefix to blank - Backward Compatibility (#16)

* update opencensus parent pom version

* update opencensus extensions for 0.19.x

* update parent pom version for confluent-extensions

* Add the capability to speed up S3 uploads using AWS transfer manager

* fix conflicting protobuf dependencies

Align protobuf dependencies to use the main pom one

* fix timestamp milliseconds in OpenCensusProtobufInputRowParser

- fix millisecond resolution being dropped when converting timestamps
- remove unnecessary conversion of ByteBuffer to ByteString
- make test code a little more concise

* improve OpenCensusProtobufInputRowParser performance (#25)

- remove the need to parse timestamps into their own column
- reduce the number of times we copy maps of labels
- pre-size hashmaps and arrays when possible
- use loops instead of streams in critical sections

Combined these changes improve parsing performance by about 15%
- added benchmark for reference

* deprecate OpenCensusInputRowParser in favor of OpenCensusProtobufInputFormat (#26)

InputRowParsers have been deprecated in favor or InputFormat.
This implements the InputFormat version of the OpenCensus Protobuf
parser, and deprecates the existing InputRowParser implementation.

- the existing InputRowParser behavior is unchanged.
- the InputFormat behaves like the InputRowParser, except for the
  default resource prefix which now defaults to "resource." instead of
  empty.
- both implementations internally delegate to OpenCensusProtobufReader,
  which is covered by the existing InputRowParser tests.

* add default query context and update timeout to 30 sec

* Setting default query lane from druid console.

* Giving more heap space for test jvm in semaphore config.

* update parent pom version for Confluent extensions

* Add Java 11 image build and remove unused MySQL images

* fix docker image build failure caused by apache#10506

* switch build to use Java 11 by default

* Fixed forbiddenapi error

* Added phases before checks

* Fixed

* OpenTelemetry Emitter Extension (#47)

Add OpenTelemetry Emitter Extension

* Add dependency check (#59)

* Add dependency check

* Fix maven-dependency-plugin errors

* Add --fail-at-end flag

* Fix comment

* METRICS-3663 OpenTelemetry Metrics InputFormat (#63)

* An OpenTelemetry metrics extension

* An InputFormat that is able to ingest metrics that are in the OpenTelemetry format

* Unit tests for the InputFormat

* Benchmarking Tests for the new OpenTelemetryMetricsProtobufInputFormat

* update parent pom version for Confluent extensions

* Adding getRequiredColumns() in our custom transforms.

* Updating shade-plugin version in opentelemetry-emitter.

* Removing the unwanted maven-shade-plugin change.

* Adding JDK version to DockerFile and removing unwanted executions from main pom.xml file. (#75)

* Passing JDK_VERSION as build args to docker build. (#76)

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

* Hybrid OpenCensusProtobufInputFormat in opencensus-extensions (#69)

* Support OpenTelemetry payloads in OpenCensusProtobufInputFormat
Support reading mixed OpenTelemetry and OpenCensus topics based on Kafka version header

* workaround classloader isolation
Workaround classloader isolation by using method handles to get access
to KafkaRecordEntity related methods and check record headers

Co-authored-by: Xavier Léauté <[email protected]>

* Modify the OpenTelemetry ProtobufReader's Handling of Attribute Types (#77)

* Only handle INT_VALUE, BOOL_VALUE, DOUBLE_VALUE and STRING_VALUE and return null otherwise
* fix wrong class in the DruidModule service provider definition

* Fixing Opencensus extension build failures.

* fix dependency check (#79)

* fix OpenTelemetry extension module service definition (#73) (#81)

* Setting default refresh value for task view as none. (#88)

As part of this we added a default parameter that can be passed for refresh widget to avoid every refresh widget getting affected.

* go/codeowners: Generate CODEOWNERS [ci skip] (#87)

* fixes in pom.xml files

* adapt to new input argument in ParseException

* adapt to the new constructor for DimensionsSpec

* update obs-data team as codeownders (#98)

* [OBSDATA-334] Patch opencensus/opentelemetry parse exception (#99)

* [METRICS-4487] add obs-oncall as codeowners (#101)

* DP-8085 - Migrate to Sempahore self-hosted agent (#100)

* [OBSDATA-334] Patch opentelemetry IllegalStateException for unsupported metric types (#103)

* Fixing checkstyle issues in openncensus and opentelemetry extensions. (#109)

* Remove SNAPSHOT from versions in confluent pom files

* Fixing CI/CD in 24.0.0 upgrade branch (#116)

* OBSDATA-440 Adding SegmentMetadataEvent and publishing them via KafkaSegmentMetadataEmitter (#117)

* Change unsupported type message from WARN to TRACE (#119)

* Use place holder for logging invalid format (#120)

Use place holder for logging invalid format for better performance

* DP-9370 - use cc-service-bot to manage Semaphore project (#118)

* chore: update repo semaphore project

* DP-9632: remediate duplicate Semaphore workflows (#121)

Only build the master branch and the `x.x.x-confluent` Druid release branches by default

* chore: update repo semaphore project

* Bump version to 24.0.1 in confluent extensions after rebasing on top of druid-24.0.1

* Bump version to 24.0.2 in confluent extensions after rebasing on top of druid-24.0.2

* OBSDATA-483: Adapt OpenCensus and OpenTelemetry extensions to the introduction of SettableByteEntity (#113)

* OBSDATA-483: Adapt opencensus extension to the introduction of SettableByteEntity

* OBSDATA-483: Adapt opentelemetry extension to the introduction of SettableByteEntity

* OBSDATA-483: Decide which reader to instantiate on read between opencensus and opentelemetry

* OBSDATA-483: Add logger config in opencensus tests

* OBSDATA-483: Fix issue with opening the byte entity

* OBSDATA-483: Instantiate the right iterator in every read request

* OBSDATA-483: Add comments

* OBSDATA-483: Address Xavier's comments

* OBSDATA-483: Remove unused member fields

* OBSDATA-483: Rename enum

* OBSDATA-483: Fix trace log to actually print the argument

* OBSDATA-483: Keep passing the underlying byte buffer and move its position explicitly

* OBSDATA-483: Fix checkstyle issues

* OBSDATA-483: Add back handling of InvalidProtocolBufferException

* OBSDATA-483: Extend the semaphore workflow execution time to 2 hours

* Revert "OBSDATA-483: Extend the semaphore workflow execution time to 2 hours"

* OBSDATA-483: Don't close iterator in sample

* chore: update repo semaphore project (#124)

Co-authored-by: Confluent Jenkins Bot <[email protected]>

* [Metrics-4776] OpenTelemetry Extensions - Upgrade otel-proto version (#125)

* Upgrade proto version
* Fix names and tests - Upgrade version
* Fix open census tests
* Fix test name

* Move to Java 17 (#128)

* bumping version of java to 17 for semaphore test run

* bumping java version to 17 as per https://github.com/confluentinc/druid/pull/127/files

* After speaking with Xavier, made these changes

* Trying to add required flags to run druid using java 17 (#130)

* Use apache-jar-resource-bundle:1.5 instead of 1.5-SNAPSHOT (apache#14054) (#131)

Co-authored-by: Tejaswini Bandlamudi <[email protected]>

* update parent pom version for Confluent extensions

* Fix CI/CD while upgrading to Druid 25.0.0

* Fix jest and prettify checks

* Adding SegmentMetadataEvent and publishing them via KafkaEmitter (apache#14281) (#139)

(cherry picked from commit 4ff6026)

* Downgrade busybox version to fix k8s IT (apache#14518) (#143)

Co-authored-by: Rishabh Singh <[email protected]>

* Passing TARGETARCH in build_args to Docker build (#144)

* Downgrade busybox version to fix k8s IT (apache#14518)

* Add TargetArch needed in distribution/Dockerfile

* Fix linting

---------

Co-authored-by: Rishabh Singh <[email protected]>

* remove docker-maven-plugin and Dockerfile customizations

- remove our custom profile to build using dockerfile-maven-plugin,
since that plugin is no longer maintained.

- remove our custom Dockerfile patches since we can now use the
  BUILD_FROM_SOURCE argument to decide if we want to build the tarball
  outside of docker.

* Revert "Trying to add required flags to run druid using java 17 (#130)" (#147)

This reverts our custom patch from commit 7cf2de4.

The necessary Java 17 exports are now included as part of 25.0.0
in https://github.com/confluentinc/druid/blob/25.0.0-confluent/examples/bin/run-java#L27-L56
which is now called by the druid.sh docker startup script as well.

The exports for java.base/jdk.internal.perf=ALL-UNNAMED are no longer
needed since apache#12481 (comment)

* removing use of semaphore cache as the public semaphore will not have cache (#145) (#148)

* utilize workflow level caching to publish the built
artifacts to the tests. otherwise turn off all caching of .m2 etc

* remove .m2/settings.xml to ensure build passes without internal artifact store

---------

Co-authored-by: Jeremy Kuhnash <[email protected]>

* OBSDATA-1365: add support for debian based base images (#149)

* Debeian based base image upgrade

* updated suggestions

* Update Dockerfile

* minor correction

---------

* Revert "fix KafkaInputFormat with nested columns by delegating to underlying inputRow map instead of eagerly copying (apache#13406) (apache#13447)" (#155)

This reverts commit 23500a4.

* Filter Out Metrics with NoRecordedValue Flag Set (#157)

Metrics that contain the NoRecordedValue Flag are being written to Druid with a 0 value. We should properly handle them in the backend

* memcached cache: switch to AWS elasticache-java-cluster-client and add TLS support  (apache#14827) (#159)

This PR updates the library used for Memcached client to AWS Elasticache Client : https://github.com/awslabs/aws-elasticache-cluster-client-memcached-for-java

This enables us to use the option of encrypting data in transit:
Amazon ElastiCache for Memcached now supports encryption of data in transit

For clusters running the Memcached engine, ElastiCache supports Auto Discovery—the ability for client programs to automatically identify all of the nodes in a cache cluster, and to initiate and maintain connections to all of these nodes.
Benefits of Auto Discovery - Amazon ElastiCache

AWS has forked spymemcached 2.12.1, and has since added all the patches included in 2.12.2 and 2.12.3 as part of the 1.2.0 release. So, this can now be considered as an equivalent drop-in replacement.

GitHub - awslabs/aws-elasticache-cluster-client-memcached-for-java: Amazon ElastiCache Cluster Client for Java - enhanced library to connect to ElastiCache clusters.
https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/services/elasticache/AmazonElastiCacheClient.html#AmazonElastiCacheClient--

How to enable TLS with Elasticache

On server side:
https://docs.aws.amazon.com/AmazonElastiCache/latest/mem-ug/in-transit-encryption-mc.html#in-transit-encryption-enable-existing-mc

On client side:
GitHub - awslabs/aws-elasticache-cluster-client-memcached-for-java: Amazon ElastiCache Cluster Client for Java - enhanced library to connect to ElastiCache clusters.

* PRSP-3603 Bump org.xerial.snappy:snappy-java to latest version to address CVEs (#164)

* Bump org.xerial.snappy:snappy-java from 1.1.8.4 to 1.1.10.5

* Add licenses

* [backport] Upgrade Avro to latest version (apache#14440) (#162)

Upgraded Avro to 1.11.1

(cherry picked from commit 72cf91f)

Co-authored-by: Tejaswini Bandlamudi <[email protected]>

* Revert "PRSP-3603 Bump org.xerial.snappy:snappy-java to latest version to address CVEs (#164)" (#166)

This reverts commit 185d655.

* Upgrade Avro to latest version to address CVEs (#167)

* OBSDATA-1697: Do not build extensions not loaded by cc-druid (#152)

Create new profiles to enable only the used extensions during the build. This helps address CVEs that were being flagged due to the unused extensions.
---------

Co-authored-by: Keerthana Srikanth <[email protected]>

* update parent pom version for Confluent extensions

* Add value to child POMs

* Upgrade dependencies to match upstream v28 & checkstyle fix

* KafkaEmitter changes

* Modifying RowFunction interface

* Fix test cases

* Fix test cases

* Fix test cases

* Fix test cases

* upgrade dependency as per druid 28

* Removing unnecessary change

* Change Maven repository URL

* Add Druid.xml

* Update tag name to match version

* Fix dist-used profile to use Hadoop compile version (#173)

* Changes based on PR comments

* Fix refreshButton

* Use onRefresh only once

* Fix snapshot so that the test passes

---------

Co-authored-by: Travis Thompson <[email protected]>
Co-authored-by: Sumit Arrawatia <[email protected]>
Co-authored-by: Xavier Léauté <[email protected]>
Co-authored-by: Apoorv Mittal <[email protected]>
Co-authored-by: Xavier Léauté <[email protected]>
Co-authored-by: Huajun Qin <[email protected]>
Co-authored-by: Huajun Qin <[email protected]>
Co-authored-by: CodingParsley <[email protected]>
Co-authored-by: Harini Rajendran <[email protected]>
Co-authored-by: Ivan Vankovich <[email protected]>
Co-authored-by: Ivan Vankovich <[email protected]>
Co-authored-by: Marcus Greer <[email protected]>
Co-authored-by: Harini Rajendran <[email protected]>
Co-authored-by: Yun Fu <[email protected]>
Co-authored-by: Xavier Léauté <[email protected]>
Co-authored-by: lokesh-lingarajan <[email protected]>
Co-authored-by: Luke Young <[email protected]>
Co-authored-by: Konstantine Karantasis <[email protected]>
Co-authored-by: Naya Chen <[email protected]>
Co-authored-by: nlou9 <[email protected]>
Co-authored-by: Corey Christous <[email protected]>
Co-authored-by: Confluent Jenkins Bot <[email protected]>
Co-authored-by: ConfluentTools <[email protected]>
Co-authored-by: Kamal  Narayan <[email protected]>
Co-authored-by: David Steere <[email protected]>
Co-authored-by: Tejaswini Bandlamudi <[email protected]>
Co-authored-by: Ghazanfar-CFLT <[email protected]>
Co-authored-by: Rishabh Singh <[email protected]>
Co-authored-by: Jeremy Kuhnash <[email protected]>
Co-authored-by: Hardik Bajaj <[email protected]>
Co-authored-by: Michael Li <[email protected]>
Co-authored-by: Keerthana Srikanth <[email protected]>
Co-authored-by: Jan Werner <[email protected]>
Co-authored-by: mustajibmk <[email protected]>
Co-authored-by: Pankaj kumar <[email protected]>
pagrawal10 pushed a commit that referenced this pull request Dec 18, 2023
pagrawal10 pushed a commit that referenced this pull request Dec 19, 2023
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