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

Static typing of metric values #185

Closed
streadway opened this Issue Apr 25, 2013 · 5 comments

Comments

Projects
None yet
4 participants
@streadway
Copy link

streadway commented Apr 25, 2013

https://github.com/prometheus/prometheus/blob/master/retrieval/format/processor0_0_1.go#L56

Is a context sensitive union type that can produce runtime errors when the "type" key does not match the expected type of the value of the "value" key. Consider using a key name for the type rather than a "type" key with the metric type as a value. Like:

[
  {"metric": {"labels":[], "count": 1} },
  {"metric": {"labels":[], "histogram": { "0.01": 15, "0.1": 20 }
]

This means that collectors ignore metrics they don't know how to join. If you want to be explicit, keep the "type" key as the tag for the union of possible metric types. Then the schema can use static typing for possible metric values.

TL;DR: Do not use interface{} types inside in the schema definition for significant fields.

@bernerdschaefer

This comment has been minimized.

Copy link
Contributor

bernerdschaefer commented Apr 26, 2013

@streadway For the new schema version, I've refactored the processor to use explicit typing. Note -- this was not a change to the schema itself, which uses type and value keys.

In theory, I would prefer <type>: <value>, but in practice I've found this style of schema to be quite painful both to product and to consume from go. Are you happy with the type checking in the pull request? Or do you want to continue discussing a schema change as well?

@streadway

This comment has been minimized.

Copy link
Author

streadway commented Apr 27, 2013

#195 looks better as a stricter context sensitive parse of union types.

I think the transfer format can be simplified even more to forego the union type (type="counter|histogram|whatever") and without needing an out-of-band version specifier.

I'll open a new issue when the thoughts are a little clearer, but the idea follows these guidelines:

http://google-styleguide.googlecode.com/svn/trunk/jsoncstyleguide.xml#Flattened_data_vs_Structured_Hierarchy

Using a top level container with the version and data fields so that export versions can be negotiated when exported over a Unix domain socket:

http://google-styleguide.googlecode.com/svn/trunk/jsoncstyleguide.xml#JSON_Structure_&_Reserved_Property_Names

And the data field containing a list of single metric type - a labeled numeric (float64) scalar with a merge strategy (3 fields).

This means that a metric can be streamed because it doesn't depend on the container and is independent of the client specific typing like Counter, Gauge or Histogram (Histogram being a class of Gauge grouping).

@matttproud

This comment has been minimized.

Copy link
Member

matttproud commented Apr 27, 2013

A few notes:

Metric type specification for human readability is an explicit design
decision, specifically to empower third-party users unfamiliar with the
domain/subject. This is something I would like to see respected for
components that humans are expected to read.

The need to handle content negotiation out-of-band is an annoyance, I will
admit. I had given some thought to a scanable format that wouldn't require
unmarshaling the entire message body for a machine to read it
incrementally. In the end, time constraints and having something good
enough (recall working on weekends and during the night seven-months to a
year ago, which didn't afford luxuries) won over the need to polish this to
death. I'm not sure that JSON-RPC's batch semantics win us anything here,
either.

I would much rather see time spent in building an optional
scanner-enabled container format that is concise and does not impose strong
library dependencies on client library implementations. Using the embedded
Protocol Buffer lite in a record streaming manner would be OK, for
instance. Just leave this up to Accepts to manage the choice of whether
JSON or Proto are sent to the collector. Boom. Both human and machine
needs can be met. Sure, I could even envision a compact text format that
would accomplish this, but we'd need to reserve some characters in the
namespace of allowed metric names and label matter to support this
reasonably.

Anyway, this is getting off topic for the PR at-hand. Please keep this in
the back of your mind.

As a side note, I generally like the Google styleguides, for they compactly
and persuasively discuss various points. This entry that was cited here
barely paid much schrift to the "why" of the recommendation, which occurs
to its detriment.
On Apr 27, 2013 10:22 AM, "Sean Treadway" notifications@github.com wrote:

#195 #195 looks better
as a stricter context sensitive parse of union types.

I think the transfer format can be simplified even more to forego the
union type (type="counter|histogram|whatever") and without needing an
out-of-band version specifier.

I'll open a new issue when the thoughts are a little clearer, but the idea
follows these guidelines:

http://google-styleguide.googlecode.com/svn/trunk/jsoncstyleguide.xml#Flattened_data_vs_Structured_Hierarchy

Using a top level container with the version and data fields so that
export versions can be negotiated when exported over a Unix domain socket:

http://google-styleguide.googlecode.com/svn/trunk/jsoncstyleguide.xml#JSON_Structure_&_Reserved_Property_Names

And the data field containing a list of single metric type - a labeled
numeric (float64) scalar with a merge strategy (3 fields).

This means that a metric can be streamed because it doesn't depend on the
container and is independent of the client specific typing like Counter,
Gauge or Histogram (Histogram being a class of Gauge grouping).


Reply to this email directly or view it on GitHubhttps://github.com//issues/185#issuecomment-17112565
.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Sep 21, 2015

JSON format 0.0.1, I think we are over it.

@fabxc fabxc closed this Sep 21, 2015

simonpasquier pushed a commit to simonpasquier/prometheus that referenced this issue Oct 12, 2017

Merge pull request prometheus#185 from ChaoticMind/trim_roadmap
Remove 'Hierarchical federation' from roadmap (it's now implemented)
@lock

This comment has been minimized.

Copy link

lock bot commented Mar 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.