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

fix: Throw better error message if version parsing fails #821

Merged
merged 4 commits into from
Aug 30, 2023

Conversation

justinkambic
Copy link
Contributor

@justinkambic justinkambic commented Aug 29, 2023

The Agent sends a request to Kibana during push operations to check its version number. Unfortunately, the API endpoint responsible for returning this data will always return a 2XX error code, even if credentials are invalid, for reasons, essentially because this endpoint acts as a heartbeat for Kibana in certain cases.

Better solution

We can get around this issue by calling /api/stats instead of /api/status. This endpoint returns the Kibana version and will throw a 401 in the event of a garbage API key, thus solving both problems.


Note: the rest of the info in the description is kept for context, but we will employ the solution explained above.

Example

# API key
abc123

# Command with incorrect key
env SYNTHETICS_API_KEY=abc12 npm run push

Explanation

Because we don't receive a 401, the Agent's internal HTTP error handling will not display any error, per its design. Unfortunately, because this issue falls through to the calling code, when we do receive an error, it is very unhelpful from a UX perspective. At this point, the user is trying to push their monitors. While waiting with their fingers crossed, hoping to see a success message, receiving a log like the example output below is confusing, disheartening, and a bad experience.

➜ npm run push

> [email protected] push
> npx @elastic/synthetics push

TypeError: Cannot read properties of undefined (reading 'number')
    at getVersion (~/lugia/node_modules/@elastic/synthetics/src/push/kibana_api.ts:138:23)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at Command.<anonymous> (~/lugia/node_modules/@elastic/synthetics/src/cli.ts:212:31)

My proposal is that the Agent should diagnose this novel error and throw a custom message in this case. The copy is by no means final and I am open to suggestions.

I'm aware that this is essentially fixing the problem by introducing tech debt. A longer term alternative fix to this approach would be to have Kibana provide a dedicated version API endpoint that could handle HTTP errors in the conventional way, but having spoken to the team, at this point it seems unlikely we will see this change in the short term. As the "fix" here has a really small footprint and is encapsulated to just this function, it seems acceptable to me, but I'm open to alternative solutions as well.

@justinkambic justinkambic added the bug Something isn't working label Aug 29, 2023
@justinkambic justinkambic self-assigned this Aug 29, 2023
Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

Code LGTM, can you add a test for the same?

src/push/kibana_api.ts Outdated Show resolved Hide resolved
@justinkambic justinkambic merged commit f585818 into main Aug 30, 2023
5 checks passed
@justinkambic justinkambic deleted the version-parse-error branch August 30, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants