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

[APM] Remove v1 and make required ECS changes #28068

Merged
merged 6 commits into from
Jan 24, 2019

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Jan 4, 2019

Closes #25536

APM Server has already moved to ECS fields. This aligns the UI with APM Server and makes master usable again.

@sorenlouv sorenlouv requested a review from a team as a code owner January 4, 2019 11:05
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv force-pushed the remove-v1-and-ecs-changes branch from e67db01 to cdeec6b Compare January 8, 2019 13:53
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv force-pushed the remove-v1-and-ecs-changes branch from cdeec6b to 96c2285 Compare January 17, 2019 17:53
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv force-pushed the remove-v1-and-ecs-changes branch from 96c2285 to 2bc99be Compare January 17, 2019 21:23
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sorenlouv sorenlouv force-pushed the remove-v1-and-ecs-changes branch from 2bc99be to 6775814 Compare January 18, 2019 09:03
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv force-pushed the remove-v1-and-ecs-changes branch from 14e3081 to 740bf96 Compare January 21, 2019 14:20
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sorenlouv sorenlouv force-pushed the remove-v1-and-ecs-changes branch 2 times, most recently from 1b8406e to 81b19f4 Compare January 21, 2019 17:36
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jasonrhodes jasonrhodes removed the request for review from ogupte January 22, 2019 16:09
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sorenlouv sorenlouv force-pushed the remove-v1-and-ecs-changes branch from a76ba0f to 7bd8798 Compare January 23, 2019 12:26
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

? FunctionWithRequiredReturnType<T>
: NonNullable<T> extends object
? DeepRequiredObject<NonNullable<T>>
: T;
Copy link
Member

Choose a reason for hiding this comment

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

What a ternary!

* DeepRequired
* Required that works for deeply nested structure
*/
type DeepRequired<T> = NonNullable<T> extends never
Copy link
Member

Choose a reason for hiding this comment

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

What does this DeepRequired mean for the idx accessor?

Copy link
Member Author

Choose a reason for hiding this comment

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

It recursively make optional properties required, so typescript won't complain when accessing props that are maybe undefined.

@@ -281,7 +281,7 @@ export function TabContent({
<Stacktrace stackframes={excStackframes} codeLanguage={codeLanguage} />
);
default:
const propData = error.context[currentTab.key] as any;
const propData = get(error, currentTab);
Copy link
Member

Choose a reason for hiding this comment

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

We can't switch to idx here because the path is dynamic, is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, idx was giving me more problems than help here so punting it for now. But I have some ideas how we can make all the properties statically typed.

? `${SPAN_HEX_ID}:"${span.span.hex_id}"`
: `${SPAN_ID}:"${span.span.id}"`;

const query = `${SPAN_ID}:"${span.span.id}"`;
Copy link
Member

Choose a reason for hiding this comment

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

I thought the "v2" schema was using the hex_id?

Copy link
Member Author

@sorenlouv sorenlouv Jan 23, 2019

Choose a reason for hiding this comment

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

Yes, it was. But hex_id has been renamed to id as part of the ECS changes (I know - so many changes...!)

Copy link
Member

Choose a reason for hiding this comment

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

Phew!

@sorenlouv
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sorenlouv sorenlouv merged commit 9d33143 into elastic:master Jan 24, 2019
@sorenlouv sorenlouv deleted the remove-v1-and-ecs-changes branch February 22, 2019 11:56
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.

4 participants