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

Split spec into multiple files #315

Merged
merged 3 commits into from
Aug 14, 2020
Merged

Conversation

felixbarny
Copy link
Member

@felixbarny felixbarny commented Aug 12, 2020

This splits the monolithic spec into smaller files. It also creates a README.md on the agents dir and renames docs/agents to specs/agents.

We started that with recent spec proposals like #307 and https://github.com/elastic/apm/pull/314/files. Mainly because the files are more coherent and smaller that way. When having the whole spec in one file, that could grow quite big. Especially going forward when we’ll hopefully have specs for all new cross-agent features.

This will cause quite a few conflicts with existing spec PRs. I want to merge this one first and then I’ll resolve the conflicts on the spec PRs. This will make it clearer what has actually changed on the spec PRs.

Preview: https://github.com/felixbarny/apm/tree/modular-spec/specs/agents

Copy link
Contributor

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

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

Generally, I like it a lot. Did you consider merging some of the shorter "chapters", e.g. spans and transactions, or exception tracking and log correlation? That would give those pages a bit more substance.

@felixbarny
Copy link
Member Author

I think the missing substance mainly means that these areas are underspecified at the moment.
For example, #314 will add more content that doesn't apply to transactions but only to spans. For that spec, I'm actually thinking about putting it in tracing-spans-discarding.md, or similar.
#307 will add more to the distributed tracing and sampling specs, #299 will add more to the transaction and span specs.

For exception tracking and log correlation, I think those topics are not related enough to be in one file.

In general, I'm more comfortable with short spec files than mixing things that are not coherent. Similar to how I'd structure source code.

@felixbarny felixbarny marked this pull request as ready for review August 14, 2020 08:35
@felixbarny felixbarny requested review from a team as code owners August 14, 2020 08:35
@felixbarny felixbarny removed the request for review from a team August 14, 2020 08:35
@felixbarny
Copy link
Member Author

felixbarny removed the request for review from elastic/apm-pm

because there are no actual changes to the spec, only it's structure

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Thanks! I think this will be much more maintainable and readable.

@felixbarny
Copy link
Member Author

Based on the many 👍 I got on slack, I'm going to merge this now so that I can rebase the other spec PRs on top of it.

@felixbarny felixbarny merged commit d8cb560 into elastic:master Aug 14, 2020
@felixbarny felixbarny deleted the modular-spec branch August 14, 2020 10:47
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