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

[TT-13917] Add uptime test implementation docs, godoc #6845

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

titpetric
Copy link
Contributor

@titpetric titpetric commented Jan 23, 2025

PR Type

Documentation, Enhancement


Description

  • Added detailed documentation for uptime test implementation.

  • Enhanced comments in UptimeTestsConfig struct for clarity.

  • Provided examples and explanations for uptime test configurations.

  • Linked related resources and highlighted outdated documentation issues.


Changes walkthrough 📝

Relevant files
Enhancement
api_definitions.go
Enhanced comments in `UptimeTestsConfig` struct                   

apidef/api_definitions.go

  • Enhanced comments for UptimeTestsConfig struct fields.
  • Clarified purpose and behavior of ExpireUptimeAnalyticsAfter.
  • Improved descriptions for ServiceDiscovery and RecheckWait.
  • +13/-3   
    Documentation
    uptime-tests.md
    Added detailed documentation for uptime tests                       

    docs/dev/uptime-tests.md

  • Added new documentation for uptime tests.
  • Included configuration examples and field explanations.
  • Linked related resources and noted outdated documentation issues.
  • Highlighted potential use cases and limitations of uptime tests.
  • +73/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    API Changes

    --- prev.txt	2025-01-23 17:45:25.045249304 +0000
    +++ current.txt	2025-01-23 17:45:20.515198396 +0000
    @@ -1349,9 +1349,19 @@
     }
     
     type UptimeTestsConfig struct {
    -	ExpireUptimeAnalyticsAfter int64                         `bson:"expire_utime_after" json:"expire_utime_after"` // must have an expireAt TTL index set (http://docs.mongodb.org/manual/tutorial/expire-data/)
    -	ServiceDiscovery           ServiceDiscoveryConfiguration `bson:"service_discovery" json:"service_discovery"`
    -	RecheckWait                int                           `bson:"recheck_wait" json:"recheck_wait"`
    +	// ExpireAnalyticsAfter controls the expire time in seconds.
    +	// If no value is configured, or if set to 0, then the expire time
    +	// is set to 100 years. This arsises out of a MongoDB restriction:
    +	//
    +	// Must have an expireAt TTL index set:
    +	// http://docs.mongodb.org/manual/tutorial/expire-data/
    +	ExpireUptimeAnalyticsAfter int64 `bson:"expire_utime_after" json:"expire_utime_after"`
    +
    +	// ServiceDiscovery configures service discovery for uptime tests.
    +	ServiceDiscovery ServiceDiscoveryConfiguration `bson:"service_discovery" json:"service_discovery"`
    +
    +	// RecheckWait is the time in seconds to wait for a re-check when a check fails.
    +	RecheckWait int `bson:"recheck_wait" json:"recheck_wait"`
     }
     
     type ValidatePathMeta struct {

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Documentation Inconsistency

    The comment for ExpireUptimeAnalyticsAfter mentions a MongoDB restriction and a TTL index requirement. Ensure this aligns with the actual implementation and usage of the field.

    // ExpireAnalyticsAfter controls the expire time in seconds.
    // If no value is configured, or if set to 0, then the expire time
    // is set to 100 years. This arsises out of a MongoDB restriction:
    //
    // Must have an expireAt TTL index set:
    // http://docs.mongodb.org/manual/tutorial/expire-data/
    ExpireUptimeAnalyticsAfter int64 `bson:"expire_utime_after" json:"expire_utime_after"`
    Outdated Documentation References

    The documentation mentions outdated fields and configurations that may no longer exist or are inconsistent with the current implementation. Verify and update these references to avoid confusion.

    Code is 2015-2017.
    
    UptimeTestsConfig:
    
    Conditions:
    - if an onhostdown event it triggered
    - if service discovery is enabled
    - initiate a reset
    - contact service discovery for all of it based on API ID.
    - use `recheck_wait` from above.
    
    HostCheckerManager uses servicediscovery, but is not clear why someone
    would need to configure it separately. As the configuration matches
    proxy.service_discovery, it would be possible to use that if not
    configured (or omitted.).
    
    The way the service discovery config is used is identical to the proxy
    service, using a shared ServiceDiscovery{} utility object. Using service
    discovery allows to coordinate healthchecks so that the testing is more
    optimal, knows to which hosts to map API IDs.
    
    Supposedly uptime tests could be aimed at a subset or a canary deployment,
    and arguably, they may not even work:
    
    Other reading:
    
    Service Discovery: https://tyk.io/docs/tyk-self-managed/#service-discovery
    
    This contains a fair bit of docs on configuring uptime tests, not the expected location.
    Search for "Conduct uptime tests" and keep reading. It also seems to document
    a fair bit of fiction as more than half of the fields don't exist.
    
    - missing `disable`, `poller_group`, ...
    - heavily outdated or we dropped a lot of it over time

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Fix typo in comment for clarity

    Correct the typo in the comment for ExpireUptimeAnalyticsAfter to replace "arsises"
    with "arises" for clarity and professionalism.

    apidef/api_definitions.go [913]

    -// This arsises out of a MongoDB restriction:
    +// This arises out of a MongoDB restriction:
    Suggestion importance[1-10]: 8

    Why: The suggestion corrects a typo in a comment, improving clarity and professionalism. While it does not impact functionality, it enhances the code's readability and documentation quality.

    8
    Fix grammatical error in documentation

    Clarify the documentation by correcting the phrase "if an onhostdown event it
    triggered" to "if an onhostdown event is triggered" for grammatical accuracy.

    docs/dev/uptime-tests.md [43]

    -- if an onhostdown event it triggered
    +- if an onhostdown event is triggered
    Suggestion importance[1-10]: 7

    Why: The suggestion fixes a grammatical error in the documentation, improving its accuracy and readability. This is important for maintaining clear and professional documentation.

    7
    Improve clarity in documentation phrasing

    Update the documentation to clarify the phrase "contact service discovery for all of
    it based on API ID" to improve readability and comprehension.

    docs/dev/uptime-tests.md [45]

    -- contact service discovery for all of it based on API ID.
    +- contact service discovery for all relevant hosts based on API ID.
    Suggestion importance[1-10]: 6

    Why: The suggestion improves the phrasing of a sentence in the documentation, enhancing its readability and comprehension. While the change is minor, it contributes to better understanding of the content.

    6

    ExpireUptimeAnalyticsAfter int64 `bson:"expire_utime_after" json:"expire_utime_after"` // must have an expireAt TTL index set (http://docs.mongodb.org/manual/tutorial/expire-data/)
    ServiceDiscovery ServiceDiscoveryConfiguration `bson:"service_discovery" json:"service_discovery"`
    RecheckWait int `bson:"recheck_wait" json:"recheck_wait"`
    // ExpireAnalyticsAfter controls the expire time in seconds.
    Copy link
    Member

    Choose a reason for hiding this comment

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

    why analytics? are these recorded in analytics and then we want to remove the entries from analytics?

    // ServiceDiscovery configures service discovery for uptime tests.
    ServiceDiscovery ServiceDiscoveryConfiguration `bson:"service_discovery" json:"service_discovery"`

    // RecheckWait is the time in seconds to wait for a re-check when a check fails.
    Copy link
    Member

    Choose a reason for hiding this comment

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

    recheck of what? is it the time between upstream requests?

    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