Skip to content
This repository has been archived by the owner on Apr 22, 2020. It is now read-only.

Writers for multiple Time series database targets #110

Closed
wants to merge 12 commits into from

Conversation

rajatparida86
Copy link
Contributor

@rajatparida86 rajatparida86 commented Feb 8, 2019

Fixes #111
Do not merge at the moment: In progress

  • Generic time series data model
  • Time series database specific writers

@zalando-robot
Copy link

Cannot start a pipeline due to:

Invalid username:
Read timed out

Click on pipeline status check Details link below for more information.

Copy link
Contributor

@alexkorotkikh alexkorotkikh left a comment

Choose a reason for hiding this comment

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

Please check code formatting through all the files. Cmd+Opt+L should help.

if (config.isLogKairosdbErrors()) {
LOG.error("KairosDB write path failed", ex);
}
dataServiceMetrics.markKairosError();
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

if (config.isLogKairosdbErrors()) {
LOG.error("KairosDB write path failed", ex);
}
dataServiceMetrics.markKairosError();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's abstract class, there should not be any DB-specific logic in here

}
}

protected abstract void store(List<GenericMetrics> metrics);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to agree on methods arrangement inside the class. The most common one is public -> protected -> package private -> private.


public void formatTimeSeriesMetrics(final CheckData checkData, List<GenericMetrics> metrics) {

if (null == config.getKairosdbTagFields() || config.getKairosdbTagFields().size() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This piece repeats the one from the constructor. Can we avoid duplication somehow?

private final Map<String, String> tags;

public GenericDataPoint(String id, Long value, Map<String, String> tags) {
this.id = id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if id here is better then name or something...

@Override
public void store(List<GenericMetrics> genericMetrics) {
log.debug("Writing to InfluxDB ...");
Timer.Context c = metrics.getM3DBTimer().time();
Copy link
Contributor

Choose a reason for hiding this comment

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

M3DB timer in InfluxDb writer...

final List<M3DbMetrics> points = new LinkedList<>();
for (GenericMetrics m : genericMetrics) {

for (GenericMetrics.GenericDataPoint dp : m.getDataPoints()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would extract transformation from GenericMetrics to M3DbMetrics to a separate method for better readability

@pitr
Copy link
Contributor

pitr commented Oct 24, 2019

won't do now

@pitr pitr closed this Oct 24, 2019
@pitr pitr deleted the multi-tsdb-writers branch October 24, 2019 07:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generic time series data model and TSDB writers
4 participants