Skip to content

chore: include support metrics in the project#4474

Merged
Colin Hicks (colinhicks) merged 7 commits into
5.5.xfrom
include-support-metrics
Feb 24, 2020
Merged

chore: include support metrics in the project#4474
Colin Hicks (colinhicks) merged 7 commits into
5.5.xfrom
include-support-metrics

Conversation

@ijuma

@ijuma Ismael Juma (ijuma) commented Feb 7, 2020

Copy link
Copy Markdown
Member

Description

ksqlDB is the only user of these classes, so include it in the project instead of consuming it as a library.

Source files were copied from Confluent Kafka's support-metrics-common module. They were updated to match ksqlDB checkstyle and to remove functionality that submits metrics to a Kafka topic (this was never used by ksqlDB).

Testing done

Tested by building a docker image and running it locally using the docker-compose file in the ksqldb.io quickstart as a template. Based on inspecting network traffic sent at server startup, there was an HTTP POST to the version-checker endpoint with the expected Avro payload.

POST /ksql/anon HTTP/1.1
api-version: phone-home-v1
Content-Length: 1155
Content-Type: multipart/form-data; boundary=Hf24SYtnRPyDwcgTVg30h6rdXnQ0-Xfw0jC
Host: version-check.confluent.io
Connection: Keep-Alive
User-Agent: Apache-HttpClient/4.5.3 (Java/1.8.0_212)
Accept-Encoding: gzip,deflate

--Hf24SYtnRPyDwcgTVg30h6rdXnQ0-Xfw0jC
Content-Disposition: form-data; name="cid"

anonymous
--Hf24SYtnRPyDwcgTVg30h6rdXnQ0-Xfw0jC
Content-Disposition: form-data; name="file"; filename="filename"
Content-Type: application/octet-stream

Objavro.schema¤{"type":"record","name":"KsqlVersionMetrics","namespace":"io.confluent.ksql.version.metrics","doc":"Represents basic metrics captured on a single ksql component","fields":[{"name":"timestamp","type":"long","doc":"Time when this data record was created on the ksql component (Unix time)."},{"name":"confluentPlatformVersion","type":["null",{"type":"string","avro.java.string":"String"}],"doc":"The version of the Confluent Platform this ksql component is running."},{"name":"ksqlComponentType","type":{"type":"string","avro.java.string":"String"},"doc":"Identifies the ksql component"},{"name":"isActive","type":"boolean","doc":"Indicates if the server was active in the last 24 hours. A server is considered active if it received a rest request or it has at least one running query."}]}ðp�ÛúÜ6&¡·ùÂ:ô¼å�5.5.0-SNAPSHOTSERVERðp�ÛúÜ6&¡·ùÂ
--Hf24SYtnRPyDwcgTVg30h6rdXnQ0-Xfw0jC--

(The ksqldb-server container was configured to send data to the insecure version-checker endpoint, so that the payload could be more easily verified)

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@ijuma
Ismael Juma (ijuma) requested a review from a team as a code owner February 7, 2020 14:17
@ijuma Ismael Juma (ijuma) changed the title Include support metrics in the project Include support metrics in the project (WIP) Feb 7, 2020
@colinhicks Colin Hicks (colinhicks) added this to the CP 5.5 milestone Feb 7, 2020
@colinhicks

Colin Hicks (colinhicks) commented Feb 7, 2020

Copy link
Copy Markdown
Contributor

I will be helping bring this home. Some follow-ups:

  • Ensure Avro-supporting bits from support metrics are copied
  • We can remove the configurations for the support metrics topic in BaseSupportConfig, as these were only relevant to Confluent Server.
  • Some tests refer to the support-metrics.confluent.io endpoint, which also applied only to Confluent Server. ksqlDB uses version-checker.confluent.io.

@ijuma

Ismael Juma (ijuma) commented Feb 7, 2020

Copy link
Copy Markdown
Member Author

Thank you Colin Hicks (@colinhicks)! A couple of tests that can also be removed:

  • proactiveSupportConfigIsValidKafkaConfig
  • canParseProactiveSupportConfiguration

* 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.
*/

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.

License should be updated to match ksqldb's template.

@colinhicks Colin Hicks (colinhicks) modified the milestones: CP 5.5, 0.8.0 Feb 7, 2020
@colinhicks

Copy link
Copy Markdown
Contributor

Apurva Mehta (@apurvam), I should rebase this onto 5.5.x, correct?

@colinhicks
Colin Hicks (colinhicks) changed the base branch from master to 5.5.x February 18, 2020 20:51
@colinhicks Colin Hicks (colinhicks) changed the title Include support metrics in the project (WIP) chore: include support metrics in the project Feb 21, 2020
@colinhicks

Copy link
Copy Markdown
Contributor

Verified metrics send as expected. See description for testing notes.

@agavra Almog Gavra (agavra) left a comment

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.

Given this is a copy of existing code I didn't give it a thorough review, just a quick scan to see that it does what it claims to 😝

@ijuma

Copy link
Copy Markdown
Member Author

Colin Hicks (@colinhicks) When merging this, please set yourself as the author since you did all the work. :)

@colinhicks
Colin Hicks (colinhicks) merged commit a80a7d8 into 5.5.x Feb 24, 2020
@colinhicks
Colin Hicks (colinhicks) deleted the include-support-metrics branch February 24, 2020 15:09
@colinhicks

Copy link
Copy Markdown
Contributor

Ismael Juma (@ijuma), I could not convince GitHub to make the squash-merge commit in my name, even after rewriting the author name in the initial commit.

If it's best to alter a80a7d8 with my name, we'll need to ask Almog Gavra (@agavra) or another maintainer to amend it – I don't have code-owner rights to this repo.

@colinhicks

Copy link
Copy Markdown
Contributor

Not sure if GH's behavior has changed here. Noting for myself in the future: squash merging PRs with a different author than the PR creator seems to require manually merging with git.

@ijuma

Copy link
Copy Markdown
Member Author

I see. One way to make this sort of work is to use co-authored-by to assign multiple authors.

Ismael Juma (ijuma) added a commit to confluentinc/kafka that referenced this pull request Feb 26, 2020
C3 no longer needs it (functionality removed):
confluentinc/blueway#2015

ksqlDB imported the relevant code into their tree:
confluentinc/ksql#4474

Reviewers: Vikas Singh <vikas@confluent.io>
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