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

Limit tables to 20 rows #1465

Merged
merged 4 commits into from
May 11, 2016
Merged

Limit tables to 20 rows #1465

merged 4 commits into from
May 11, 2016

Conversation

2opremio
Copy link
Contributor

Should help with #1457 and #1454 until we have #985

@2opremio 2opremio force-pushed the truncate-container-envs branch from db91a52 to acc4070 Compare May 10, 2016 14:14
@2opremio 2opremio changed the title [WIP] Limit container env table size to 20 [WIP] Limit tables to 20 rows May 10, 2016
@2opremio 2opremio force-pushed the truncate-container-envs branch from acc4070 to 7a8e614 Compare May 10, 2016 14:46
@2opremio
Copy link
Contributor Author

2opremio commented May 10, 2016

@davkal Here's a sample report with the new truncation field in place: report_truncation.json.gz

@2opremio 2opremio force-pushed the truncate-container-envs branch from 925ad63 to 0a62080 Compare May 10, 2016 22:11
@rade
Copy link
Member

rade commented May 10, 2016

What gets truncated by this besides container env vars?

@rade
Copy link
Member

rade commented May 10, 2016

What gets truncated by this besides container env vars?

AFAICT it is

  • docker image labels
  • docker container labels
  • k8s labels

@2opremio
Copy link
Contributor Author

That is correct:

scope/probe$ grep -r AddTable *
docker/container.go:    result = result.AddTable(LabelPrefix, c.container.Config.Labels)
docker/container.go:    result = result.AddTable(EnvPrefix, c.env())
docker/reporter.go:             node = node.AddTable(ImageLabelPrefix, image.Labels)
kubernetes/meta.go:     }).AddTable(LabelPrefix, m.Labels())

@2opremio 2opremio force-pushed the truncate-container-envs branch from 0a62080 to 100f78d Compare May 11, 2016 09:50
@2opremio 2opremio changed the title [WIP] Limit tables to 20 rows Limit tables to 20 rows May 11, 2016
@2opremio
Copy link
Contributor Author

@davkal Looking good

Two comments:

  • Can we make it so that the message is shown when clicking not just hover?
  • Maybe the copy could be slightly refined.
    Currently: The section Environment Variables has been truncated because of too many entries.
    714 entries are not shown. We are working on making this better.

I would propose This section is too long and has been truncated (714 extra entries not included). We are working to remove this limitation.

I think it's more to the point and the section is obvious since it's name is right next to the warning sign.

@2opremio
Copy link
Contributor Author

@paul @foot Can you PTAL ?

@paulbellamy
Copy link
Contributor

paulbellamy commented May 11, 2016

I'd even drop We are working on making this better. Maybe just 714 entries hidden (which is all we say about filtered unconnected processes).

Maybe, yeah. I wouldn't use hidden or not displayed because it can cause confusion with the table expansion.

@paulbellamy
Copy link
Contributor

backend code, LGTM.

@foot
Copy link
Contributor

foot commented May 11, 2016

Code LGTM!

Copy about why we're truncating is kind of nice, explaining the situation a bit. But there are probably reasons not too. Like admission of imperfection in product or something.

I like

We are working to remove this limitation.

@foot
Copy link
Contributor

foot commented May 11, 2016

Should host.process-list/container-list be getting truncated? Those are 500+ on the attached report and make the UI a bit janky.

@2opremio
Copy link
Contributor Author

@foot That's tricker because ordering processes by CPU use etc ... require having them all to be useful.

@2opremio
Copy link
Contributor Author

LGTM

@davkal davkal merged commit 715003e into master May 11, 2016
@davkal davkal deleted the truncate-container-envs branch May 11, 2016 15:14
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.

5 participants