-
Notifications
You must be signed in to change notification settings - Fork 4
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
Provide consistent interface for Synthetics Tests API #103
Provide consistent interface for Synthetics Tests API #103
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to get your feedback to my comments before merging the code. The code is good to go in as long as we've tickets for:
- support for labels for tests and agents
- support for passing
edate
inTestUpdate
requests
Status: syntheticspb.TestStatus(syntheticspb.TestStatus_value[string(t.Status)]), | ||
Settings: ts, | ||
Cdate: nil, // read-only | ||
Edate: nil, // read-only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to support the ability to provide edate
in TestUpdate
requests. If provided, the API returns error if the object to be modified has been modified in the DB after the edate
timestamp. This allows to guard against concurrent modifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: faa5fc9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, minor comments left
GetTestResults() and GetTestTrace() methods will be implemented in next PR. Issue: KNTK-399
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting ever better.
examples/synthetics_example_test.go
Outdated
@@ -55,9 +70,10 @@ func demonstrateSyntheticsAgentAPI() error { | |||
fmt.Println("Number of agents:", len(getAllResp.Agents)) | |||
fmt.Println("Number of invalid agents:", getAllResp.InvalidAgentsCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be printed in large blinking red, if the value is non-zero :). The number is actually of any use for Kentik engineers. A non-zero value indicates some bug in the Kentik system. Let's print it only if it is non-zero and add text suggesting to contact Kentik support. Something like: Kentik API returned <N> invalid agents. Please, contact Kentik support.
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 061b51c
Skipped the blinking red part ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And fixed again: e9566f4
examples/synthetics_example_test.go
Outdated
fmt.Println("Number of invalid tests:", getAllResp.InvalidCount) | ||
fmt.Println() | ||
fmt.Println("Number of tests:", len(getAllResp.Tests)) | ||
fmt.Println("Number of invalid tests:", getAllResp.InvalidTestsCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like for agents, this number is of any use only to Kentik engineers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: e9566f4
examples/synthetics_example_test.go
Outdated
return fmt.Errorf("client.Synthetics.GetAllAgents: %w", err) | ||
} | ||
|
||
dualIPRustAgentID, err := pickDualIPRustAgentID(getAllResp.Agents) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that the goal of the example is to demonstrate the Go SDK (as opposed to test the Kentik API), let's use only IPv4 agents and tests. The reason for this is that the API is little bit behind the development of the Kentik synthetics system, and it currently cannot reliably represent dual stacked agents (it has no place to show the "other" IP address. Which one (v4 or v6) of addresses is shown in the ip
(and also local_ip
) property is undefined. The system internally (since about 2 months ago) does the right thing, but it is not visible in the API. The visible behavior may confuse more detailed oriented customers :).
We can add examples of choosing dual stack agents and picking protocol family in the test once the API catches up and we add support for new fields in the SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that soon (PR is pending review) node.js (ksynth-agent
) agents will be allowed to run any synthetic test. Rust (ksynth
) agents will never be able to run page-load
and transaction
tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified example to use IPv4 agents: 695420c
Let us know when changes to agents arrive: we'll update docstrings and simplify TestDemonstrateSyntheticsTestsAPI_CreateMinimalTests
@mateuszmidor @mmac-m3a All comments addressed |
GetTestResults() and GetTestTrace() methods will be implemented in next PR.
Based-on: #104
Issue: KNTK-399