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

Option to disable stream api checking in Nginx Plus #7241

Merged
merged 42 commits into from
Aug 20, 2020

Conversation

szibis
Copy link
Contributor

@szibis szibis commented Jul 30, 2020

Add feature to disable stream Nginx Plus API if no stream in Nginx configuration

With the current integration version, we get a lot of errors in Nginx Plus for calling Stream endpoints stats that does not exist if no Stream defined in Nginx configuration.

Now we will have the ability to disable stream checking in the Datadog integration configuration file.

When use_plus_api: True then we need now set also use_plus_api_stream: True to have all covered. This is aligned to values defined for endpoints in the Nginx integration code.

    ## @param use_plus_api_stream - boolean - optional - default: false
    ## If you are using the commercial version of Nginx, for releases R13 and above,
    ## there is a new API to replace the extended status API.
    ## See https://www.nginx.com/blog/nginx-plus-r13-released/
    ## When no Stream servers used we can disable this part to remove errors in Nginx Plus API calls.
    ## To use it set `use_plus_api_stream` to true
    #
    # use_plus_api_stream: true
  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@szibis szibis requested review from a team as code owners July 30, 2020 14:10
@codecov
Copy link

codecov bot commented Jul 30, 2020

Copy link
Contributor

@hithwen hithwen left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your contribution. The PR looks good overall the only caveat is we need to be retrocompatible and because of that the default value need to be True

nginx/assets/configuration/spec.yaml Show resolved Hide resolved
nginx/README.md Outdated Show resolved Hide resolved
szibis and others added 2 commits August 3, 2020 16:10
Co-authored-by: Julia <611228+hithwen@users.noreply.github.com>
Co-authored-by: Julia <611228+hithwen@users.noreply.github.com>
hithwen
hithwen previously approved these changes Aug 3, 2020
@hithwen
Copy link
Contributor

hithwen commented Aug 3, 2020

Hi, you need to sync the config file again, otherwise its good to go

Copy link
Contributor

@hithwen hithwen left a comment

Choose a reason for hiding this comment

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

Needs green build

@szibis
Copy link
Contributor Author

szibis commented Aug 10, 2020

@hithwen how can I sync config file once again ??

@hithwen
Copy link
Contributor

hithwen commented Aug 10, 2020

@szibis you need to update to the latest version of ddev

@szibis szibis requested a review from a team as a code owner August 10, 2020 16:53
@szibis szibis requested review from hithwen and removed request for a team August 10, 2020 17:44
@hithwen
Copy link
Contributor

hithwen commented Aug 11, 2020

I'm very sorry, yesterday we removed some trailing whitespaces from the base templates so you need to rebase with master before syncing (or try applying the suggestion I've left)

nginx/README.md Outdated Show resolved Hide resolved
nginx/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
nginx/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
nginx/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
nginx/datadog_checks/nginx/data/conf.yaml.example Outdated Show resolved Hide resolved
nginx/datadog_checks/nginx/data/conf.yaml.example Outdated Show resolved Hide resolved
nginx/datadog_checks/nginx/data/conf.yaml.example Outdated Show resolved Hide resolved
nginx/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
nginx/datadog_checks/nginx/data/conf.yaml.example Outdated Show resolved Hide resolved
szibis and others added 12 commits August 20, 2020 09:56
Co-authored-by: Julia <611228+hithwen@users.noreply.github.com>
Co-authored-by: Kari Halsted <12926135+kayayarai@users.noreply.github.com>
Co-authored-by: Kari Halsted <12926135+kayayarai@users.noreply.github.com>
Co-authored-by: Kari Halsted <12926135+kayayarai@users.noreply.github.com>
Co-authored-by: Kari Halsted <12926135+kayayarai@users.noreply.github.com>
Co-authored-by: Kari Halsted <12926135+kayayarai@users.noreply.github.com>
Co-authored-by: Kari Halsted <12926135+kayayarai@users.noreply.github.com>
Co-authored-by: Kari Halsted <12926135+kayayarai@users.noreply.github.com>
Co-authored-by: Kari Halsted <12926135+kayayarai@users.noreply.github.com>
Co-authored-by: Kari Halsted <12926135+kayayarai@users.noreply.github.com>
Co-authored-by: Julia <611228+hithwen@users.noreply.github.com>
@szibis szibis requested a review from hithwen August 20, 2020 08:10
Co-authored-by: Julia <611228+hithwen@users.noreply.github.com>
@hithwen hithwen merged commit 0e62512 into DataDog:master Aug 20, 2020
@szibis szibis deleted the nginx_plus_no_stream branch August 20, 2020 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants