refactor raintank/schema into a v2 version #15

woodsaj · 2018-02-22T21:23:09Z

reuse the same MetricMetadata struct in MetricData and MetricDefinition
remove "Metric" field from the MetricMetadata
Update SetId() to use the Name field instead of Metric.
update MetricData to be a combo of MetricMetadata and a Point
Add a MetricPoint struct, which is just the metricId + a Point
Remove non-zero interval requirement in validation.
add MetricWithMetadata and DataPoint interfaces
- MetricData and MetricDefinition both implement MetricWithMetaData
- MetricData and MetricPoint both implement DataPoint
Add new msg.Formats for FormatMetricDataArrayJson and FormatMetricDataArrayMsgp
add new msg.Formats for FormatMetricData and FormatMetricPoint which are for messages sent to and received from kafka
Add EncodeDataPoint and DecodeDatapoint functions
Add EncodeMetricDataArray and DecodeMetricDataArray which are backwards compatible for schema.v1 payloads
use a bufferPool for SetId as a byte slice is used to get the md5sum then discarded.

replay · 2018-02-23T06:15:48Z

metric.go

+}
+
+// returns a id (hash key) in the format OrgId.md5Sum
+// the md5sum is a hash of the the concatination of the


"concatenation"

replay · 2018-02-23T06:18:46Z

metric.go

+	buffer.WriteString(m.Unit)
+	buffer.WriteByte(0)
+	buffer.WriteString(m.Mtype)
+	buffer.WriteByte(0)


I know the old version before this patch did the same, but anyway: Isn't that one buffer.WriteByte(0) too many? If there are no tags then the last char of buffer is 0, if there are >0 tags then there's a double-0 before the first tag.
I guess we can't change that, otherwise that would lead to trouble because the IDs of existing metrics would change with the update.

replay · 2018-02-23T06:22:31Z

metric_test.go

+			Interval: 15,
+			Unit:     "gauge",
+			Mtype:    "ms",
+			Tags:     []string{"key1:val1", "key2:val2"},


The tag format is <key>=<value>

replay · 2018-02-23T06:22:47Z

metric_test.go

+			Interval: 15,
+			Unit:     "gauge",
+			Mtype:    "ms",
+			Tags:     []string{"key1:val1", "key2:val2"},


The tag format is <key>=<value>

replay · 2018-02-23T06:28:20Z

msg/msg.go

@@ -91,3 +40,136 @@ func CreateMsg(metrics []*schema.MetricData, id int64, version Format) ([]byte,
 	}
 	return buf.Bytes(), nil
 }
+
+func DecodeMetricDataArray(b []byte) ([]*schema.MetricData, error) {
+	if len(b) < 9 {


a comment where the number 9 comes from would be helpful

replay · 2018-02-23T06:39:33Z

msg/msg.go

+		}
+		return p, nil
+	default:
+		// if format is not known, lets assume that this is a MetricData message with no version byte


by handling it like this, isn't there a risk that we miss a potential error case and end up inserting garbage data into the backend store because the received metrics were somehow corrupted and we didn't notice it due to this default case?
If this default case would just do nothing and error we'd notice corrupted input faster probably

Messages currently being sent into kafka dont have a format byte. So we have no choice other then to treat messages like this.

if the only new message type is the compact payload i proposed in grafana/metrictank#849, then we can also rely on the length of the message for additional confidence. (the proposed payload is exactly 28 bytes)

replay · 2018-02-23T06:48:28Z

metric.go

+
+// returns a id (hash key) in the format OrgId.md5Sum
+// the md5sum is a hash of the the concatination of the
+// metric + each tag key:value pair (in metrics2.0 sense, so also fields), sorted alphabetically.


- reuse the same MetricMetadata struct in MetricData and MetricDefinition - remove "Metric" field from the MetricMetadata - Update SetId() to use the Name field instead of Metric. - update MetricData to be a combo of MetricMetadata and a Point - Add a MetricPoint struct, which is just the metricId + a Point - Remove non-zero interval requirement in validation. - add MetricWithMetadata and DataPoint interfaces - MetricData and MetricDefinition both implement MetricWithMetaData - MetricData and MetricPoint both implement DataPoint - Add new msg.Formats for FormatMetricDataArrayJson and FormatMetricDataArrayMsgp - add new msg.Formats for FormatMetricData and FormatMetricPoint which are for messages sent to and received from kafka - Add EncodeDataPoint and DecodeDatapoint functions - Add EncodeMetricDataArray and DecodeMetricDataArray which are backwards compatible for schema.v1 payloads - use a bufferPool for SetId as a byte slice is used to get the md5sum then discarded.

Dieterbe

I like the embedding of different structs within one another. that seems useful. i need to look into the format stuff a bit more

Dieterbe · 2018-03-15T07:36:35Z

bufferpool.go

+
+type BufferPool struct {
+	pool sync.Pool
+}


what's the purpose of embedding the sync.Pool as opposed to simply type aliasing a BufferPool to sync.Pool, or even skip the BufferPool type and have NewBufferPool return a *sync.Pool ?

Dieterbe · 2018-03-15T07:59:26Z

metric.go

 	if m.OrgId == 0 {
 		return errInvalidOrgIdzero
 	}
-	if m.Interval == 0 {
-		return errInvalidIntervalzero
-	}


we have now decided to not support interval autodetection any time soon, so we can add this validation back

Dieterbe · 2018-03-15T11:33:59Z

metric.go

-	b = append(b, []byte(m.Name)...)
-	return b
+type MetricPoint struct {
+	Id string


I thought we agreed in #199 that Id's should be stored as a [16]byte (and a uint32 for orgId when needed)

Dieterbe · 2018-03-15T11:36:49Z

metric.go

 	buffer.WriteByte(0)
 	buffer.WriteString(m.Unit)
 	buffer.WriteByte(0)
 	buffer.WriteString(m.Mtype)
 	buffer.WriteByte(0)
 	fmt.Fprintf(buffer, "%d", m.Interval)

-	for _, t := range m.Tags {
-		if len(t) >= 5 && t[:5] == "name=" {


why remove this clause?

Dieterbe · 2018-03-15T11:58:35Z

Add new msg.Formats for FormatMetricDataArrayJson and FormatMetricDataArrayMsgp

huh? these formats exist also in master. am I missing something?

big picture though, we should stop using messagepack. it takes too much space. I thought we established that in the discussions of #199 and I think grafana/metrictank#849 (comment) confirms it.

woodsaj requested review from Dieterbe, replay and DanCech February 22, 2018 21:23

woodsaj force-pushed the v2 branch from 7b26621 to cd1e7e5 Compare February 22, 2018 21:28

replay reviewed Feb 23, 2018

View reviewed changes

woodsaj force-pushed the v2 branch from cd1e7e5 to dcdcd46 Compare February 23, 2018 09:58

woodsaj mentioned this pull request Mar 15, 2018

WIP: Automatic detection of metric interval grafana/metrictank#849

Closed

Dieterbe suggested changes Mar 15, 2018

View reviewed changes

DanCech closed this Jan 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor raintank/schema into a v2 version #15

refactor raintank/schema into a v2 version #15

woodsaj commented Feb 22, 2018 •

edited

Loading

replay Feb 23, 2018

replay Feb 23, 2018 •

edited

Loading

replay Feb 23, 2018

replay Feb 23, 2018

replay Feb 23, 2018

replay Feb 23, 2018 •

edited

Loading

woodsaj Feb 23, 2018

Dieterbe Mar 15, 2018

replay Feb 23, 2018

Dieterbe left a comment

Dieterbe Mar 15, 2018

Dieterbe Mar 15, 2018

Dieterbe Mar 15, 2018

Dieterbe Mar 15, 2018

Dieterbe commented Mar 15, 2018

refactor raintank/schema into a v2 version #15

refactor raintank/schema into a v2 version #15

Conversation

woodsaj commented Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

replay Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

replay Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dieterbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dieterbe commented Mar 15, 2018

woodsaj commented Feb 22, 2018 •

edited

Loading

replay Feb 23, 2018 •

edited

Loading

replay Feb 23, 2018 •

edited

Loading