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 parameterizable duration to retrieve data from prometheus #177

Merged
merged 5 commits into from
Jul 1, 2020
Merged

add parameterizable duration to retrieve data from prometheus #177

merged 5 commits into from
Jul 1, 2020

Conversation

Nexucis
Copy link
Contributor

@Nexucis Nexucis commented Jun 26, 2020

Note: that's the last thing missing to be able to fix back the PR on prometheus side

@Nexucis Nexucis requested a review from slrtbtfs June 26, 2020 11:57
Copy link
Member

@slrtbtfs slrtbtfs left a comment

Choose a reason for hiding this comment

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

Ideally config.Interval should allow formats such as "1w" and be stored as a time.Duration.

It should be possible to set it from the client side over the DidChangeConfiguration API.

Also a more descriptive name that indicates what it's used for would be nice.

langserver/config.go Outdated Show resolved Hide resolved
@Nexucis
Copy link
Contributor Author

Nexucis commented Jun 26, 2020

For the name, yeah I'm agree it's not a super good one. But so far I don't have better. If you have a better one I will replace it :).
Maybe the weekend will help to find it.

yeah why not to have a duration instead. Good idea

@slrtbtfs
Copy link
Member

How about MetadataLookbackInterval?

@Nexucis
Copy link
Contributor Author

Nexucis commented Jun 26, 2020

Here we go, it should be a bit better now

@Nexucis Nexucis requested a review from slrtbtfs June 26, 2020 21:53
Copy link
Member

@slrtbtfs slrtbtfs left a comment

Choose a reason for hiding this comment

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

Aside from the things mentioned in the review, adding this new config option will also require updating the package.json of the VS Code extension.

langserver/config.go Outdated Show resolved Hide resolved
langserver/config.go Show resolved Hide resolved
langserver/config.go Outdated Show resolved Hide resolved
@slrtbtfs
Copy link
Member

We could also store the look back interval in the metadataService instance and change it the same way URLs are changed.

This keeps complexity out of the langserver package and prevents the race condition mentioned above.

@Nexucis
Copy link
Contributor Author

Nexucis commented Jun 27, 2020

I'm not really in favor of doing the same than the prometheus URL. It doesn't require the same completexity and I prefer that the metadataService is opened to use freely the from/start value instead of passing by an attribute.

@Nexucis Nexucis requested a review from slrtbtfs June 27, 2020 09:46
@slrtbtfs
Copy link
Member

slrtbtfs commented Jun 30, 2020

  1. So first I think I will forget you were implying I am lazy.

Sorry for having worded this in an offensive way.

I feel like I should clarify a few things here:

  • "laziness" in the sense of "avoiding effort" is often a legitimate reason to not do something, especially in the context of a voluntary activities.
  • The concept of "laziness" as a character flaw is quite problematic, since it's often used to blame and shame people for what might be the symptoms of mental health issues or invisible disabilities. From individuals with executive dysfunction caused by ADHD or ASD there are reports of this contributing to a negative self image and depression. So I should have known better than to imply it make use of that concept in any context.
  • Since this doesn't really come across in PR discussions that focus on what problems are still to be addressed until something can be merged: Your contributions to this project consistently have been of high quality and are very much appreciated.

@Nexucis
Copy link
Contributor Author

Nexucis commented Jul 1, 2020

here we go, this one should be fine now.

It will remain to update the client of the langserver after that. And then it will need to be released in order to be used in Prometheus.

But otherwise normally that should be the end of the changes requested in order to integrate it into Prometheus.

Copy link
Member

@slrtbtfs slrtbtfs left a comment

Choose a reason for hiding this comment

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

Thanks!

@slrtbtfs slrtbtfs merged commit c20ca98 into prometheus-community:master Jul 1, 2020
@Nexucis Nexucis deleted the feature/duration branch July 2, 2020 11:07
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