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

Add methods to model.Duration for JSON marshalling and unmarshalling #280

Merged
merged 2 commits into from
Mar 9, 2021
Merged

Add methods to model.Duration for JSON marshalling and unmarshalling #280

merged 2 commits into from
Mar 9, 2021

Conversation

56quarters
Copy link
Contributor

@56quarters 56quarters commented Mar 5, 2021

Add methods to model.Duration structs to allow them to be serialized
and unserialized to/from JSON. This helps downstream consumers of the
structs that attempt to use them in objects that are expected to be
able to be represented as JSON (such as Cortex).

An example in Cortex where this would help: validation.Limits

Signed-off-by: Nick Pillitteri nick.pillitteri@grafana.com

Add methods to model.Duration structs to allow them to be serialized
and unserialized to/from JSON. This helps downstream consumers of the
structs that attempt to use them in objects that are expected to be
able to be represented as JSON (such as Cortex).

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters closed this Mar 5, 2021
@56quarters 56quarters reopened this Mar 5, 2021
@roidelapluie
Copy link
Member

I am wondering why this works in Prometheus rules (yaml) but not json ?

@56quarters
Copy link
Contributor Author

@roidelapluie I'm not sure I understand your question.

@roidelapluie
Copy link
Member

Prometheus has an API page where we expose rules, it can also unmarshal this as yaml.

However, we do not implement yamlUnmarshaller here .. so I wonder why json marshalling would be mandatory and not yaml marshalling

@56quarters
Copy link
Contributor Author

56quarters commented Mar 5, 2021

YAML marshalling and unmarshalling (as well as text marshalling/unmarshalling) is implemented for this type in the same file:

func (d Duration) MarshalYAML() (interface{}, error) {

@roidelapluie
Copy link
Member

Oh sorry I did not see it 👀

LGTM except one thing: can you make all the unmarshal functions look the same (look at how we manage the errors)? Just pick one patterm.

@56quarters
Copy link
Contributor Author

LGTM except one thing: can you make all the unmarshal functions look the same (look at how we manage the errors)? Just pick one patterm.

Sure, will do

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters
Copy link
Contributor Author

@roidelapluie Changes made but the CI seems to have failed for unrelated reasons. Thanks for all your help on this!

@roidelapluie roidelapluie merged commit 3ebd397 into prometheus:main Mar 9, 2021
@roidelapluie
Copy link
Member

Thanks!

@roidelapluie
Copy link
Member

@56quarters do you need a release for this?

@56quarters
Copy link
Contributor Author

@roidelapluie That'd be great if it's not too much trouble, thanks!

@roidelapluie
Copy link
Member

v0.19.0 is out :)

This was referenced Mar 12, 2021
This was referenced Mar 15, 2021
bboreham added a commit to cortexproject/cortex that referenced this pull request Mar 23, 2021
Now we only need to make a copy if GetRef() returns zero

Note Prometheus update brings in JSON marshalling of model.Duration
prometheus/common#280

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
bboreham added a commit to cortexproject/cortex that referenced this pull request Mar 24, 2021
Now we only need to make a copy if GetRef() returns zero

Note Prometheus update brings in JSON marshalling of model.Duration
prometheus/common#280

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
bboreham added a commit to cortexproject/cortex that referenced this pull request Mar 26, 2021
Now we only need to make a copy if GetRef() returns zero

Note Prometheus update brings in JSON marshalling of model.Duration
prometheus/common#280

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
bboreham added a commit to cortexproject/cortex that referenced this pull request Mar 30, 2021
Now we only need to make a copy if GetRef() returns zero

Note Prometheus update brings in JSON marshalling of model.Duration
prometheus/common#280

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
pracucci pushed a commit to cortexproject/cortex that referenced this pull request Mar 31, 2021
* Add BenchmarkIngesterV2Push

So we can check the efficiency of changes to `v2Push()`.
Broadly copied from `BenchmarkIngesterPush()`.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

* Use TSDB's index of series, remove RefCache

prometheus/prometheus#8600 adds a method to
`TSDB.Appender` which allows us to save building
a parallel cache, reducing ingester heap by about 20%.

We depend on values from GetRef() remaining valid while v2Push() uses
them. Currently the only way a ref can be invalidated is by a head
compaction, which cannot happen while v2Push() holds the append lock.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

* New version of GetRef() that returns labels

Now we only need to make a copy if GetRef() returns zero

Note Prometheus update brings in JSON marshalling of model.Duration
prometheus/common#280

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

* Add comment on use of copiedLabels

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

* Update to Prometheus main branch

Pinned gRPC and other dependencies changed by the update in Prometheus
to avoid taking so much change on this PR.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
tomwilkie pushed a commit to grafana/mimir that referenced this pull request Jul 13, 2021
* Add BenchmarkIngesterV2Push

So we can check the efficiency of changes to `v2Push()`.
Broadly copied from `BenchmarkIngesterPush()`.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

* Use TSDB's index of series, remove RefCache

prometheus/prometheus#8600 adds a method to
`TSDB.Appender` which allows us to save building
a parallel cache, reducing ingester heap by about 20%.

We depend on values from GetRef() remaining valid while v2Push() uses
them. Currently the only way a ref can be invalidated is by a head
compaction, which cannot happen while v2Push() holds the append lock.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

* New version of GetRef() that returns labels

Now we only need to make a copy if GetRef() returns zero

Note Prometheus update brings in JSON marshalling of model.Duration
prometheus/common#280

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

* Add comment on use of copiedLabels

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

* Update to Prometheus main branch

Pinned gRPC and other dependencies changed by the update in Prometheus
to avoid taking so much change on this PR.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Former-commit-id: ad3ea42
alanprot pushed a commit to alanprot/common that referenced this pull request Mar 15, 2023
Enable Prometheus native histograms for request_duration_seconds
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.

None yet

2 participants