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

KIP 714 with compression support #4721

Conversation

anchitj
Copy link
Member

@anchitj anchitj commented May 17, 2024

No description provided.

milindl and others added 30 commits July 25, 2023 12:06
* WIP:Push telemetry is being scheduled now.

* Push

* Compliation Fix

* Working

* Use UUID in PUSH

* Remove fprintf

* Changes

* Fix CONFIGURATION.md

* Fix size

* Update s2i bounds

---------

Co-authored-by: Milind L <miluthra@confluent.io>
* Add broker selection and client termination

* Address review comments
* Serialise metrics using nanopb

* Move nanopb and opentelemetry inside src

* Add metrics.options file

* Remove unused includes

* Style fix

* Fix formatting

* Skip copyright check

* Add nanopb and opentelemetry in windows vcxproj

* Include headers directories in CMAKE

* Use flexver with PushTelemetry

* Fix memory leaks

* Change import path

* Use rd_bool_t everywhere

* Fix librdkafka.vcxproj

* Use rd_bool_t

* PR Feedback

* Add nanopb license

* Include opentelemtry license
* Support for delta temporality

* Style fix

* Fix bugs

* Fix memory leaks and formatting

* Fixes

* PR Feedback
* Add telemetry encode and decode unit tests

* Style fix

* Improve test

* PR Feedback
* Add max telemetry bytes

* Clear telemetry_max_bytes

* PR comments
Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Comments about encode and decode

src/rdkafka_op.h Outdated Show resolved Hide resolved
src/rdkafka_telemetry_encode.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry_encode.c Show resolved Hide resolved
src/rdkafka_telemetry_encode.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry_decode.h Outdated Show resolved Hide resolved
src/rdkafka_telemetry_encode.c Outdated Show resolved Hide resolved
Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Most of remaining comments

src/rdkafka_broker.c Show resolved Hide resolved
src/rdkafka_int.h Outdated Show resolved Hide resolved
src/rdkafka_mock_handlers.c Outdated Show resolved Hide resolved
src/rdkafka_mock_handlers.c Show resolved Hide resolved
src/rdkafka_mock_handlers.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry.c Show resolved Hide resolved
src/rdkafka_telemetry.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry.c Outdated Show resolved Hide resolved
Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Comments about unit tests and naming

src/rdkafka_telemetry_decode.c Show resolved Hide resolved
src/rdkafka_telemetry_decode.c Show resolved Hide resolved
src/rdkafka_telemetry_decode.c Show resolved Hide resolved
src/rdkafka_telemetry_decode.c Show resolved Hide resolved
src/rdkafka_telemetry_decode.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry_decode.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry_encode.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry_encode.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry_encode.c Outdated Show resolved Hide resolved
Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Copyright and includes comments

src/rd.h Outdated Show resolved Hide resolved
src/rdkafka_telemetry_decode.c Show resolved Hide resolved
src/rdkafka_telemetry_decode.h Outdated Show resolved Hide resolved
#ifndef _RDKAFKA_RDKAFKA_TELEMETRY_ENCODE_H
#define _RDKAFKA_RDKAFKA_TELEMETRY_ENCODE_H

#include "rdtypes.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Add #include "rdkafka_int.h" as it's needed for rd_kafka_t

Suggested change
#include "rdtypes.h"
#include "rdkafka_int.h"
#include "rdtypes.h"

src/rdkafka_telemetry_encode.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry.c Outdated Show resolved Hide resolved


#ifndef _RD_KAFKA_TELEMETRY_H_
#define _RD_KAFKA_TELEMETRY_H_
Copy link
Contributor

Choose a reason for hiding this comment

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

Required for rd_kafka_t

Suggested change
#define _RD_KAFKA_TELEMETRY_H_
#define _RD_KAFKA_TELEMETRY_H_
#include "rdkafka_int.h"

src/rdkafka_telemetry.h Outdated Show resolved Hide resolved
src/rdkafka_telemetry_decode.c Outdated Show resolved Hide resolved
@emasab
Copy link
Contributor

emasab commented Jul 7, 2024

Please add these to the list of Supported protocol versions in INTRODUCTION.md

| 71      | GetTelemetrySubscriptions     | 0          | 0              |
| 72      | PushTelemetry                 | 0          | 0              |

Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Some comments about placeholders types in formatted strings

src/rdkafka_telemetry_decode.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry_encode.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry_encode.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry.c Outdated Show resolved Hide resolved
src/rdkafka_telemetry.c Outdated Show resolved Hide resolved
@anchitj anchitj force-pushed the dev_kip_714_mock_broker_integration_tests_master_merge_with_compression branch from 9616eba to 4732ff6 Compare July 8, 2024 06:44
Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks @anchitj and @milindl !

@anchitj anchitj merged commit 6eaf89f into master Jul 8, 2024
2 checks passed
@anchitj anchitj deleted the dev_kip_714_mock_broker_integration_tests_master_merge_with_compression branch July 8, 2024 07:47
@georgthegreat
Copy link

Hmm...
This PR introduced a mandatory dependency on nanopb runtime and opentelemetry protos generated with nanopb.
While there is nothing wrong with either of dependencies, having a switch to disable them would be nice.

@anchitj @emasab what do you think?
Other depenndencies are configurable.

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.

5 participants